Create TTTAttributedLabelLink class, and use it to manage default/active/inactive link state and accessibility values for non-default link types

[getaaron]

• Create TTTAttributedLabelLink class, and use it to manage default/active/inactive link state and accessibility values for non-default link types
• Updates example project to compile properly with this new class
• Update documentation

[mattt]

Thanks for your work on this, @getaaron. Some quick feedback:

• TTTAttributedLabelLink should be part of TTTAttributedLabel.{h,m}. Since there are no situations in which it would be used without the label itself, extracting into a separate file is to its net disadvantage.
• Is there any particular reason that self.links is being renamed to self.linkModels?
• No direct initializer for TTTAttributedLabelLink should be publicly exposed. Instead, links should only ever be initialized as a consequence of methods that add links to the label.
• Further, addLink... methods should return an instance of the link created by the method, rather than void. This would be helpful for keeping track of links without an associated text checking result.
• A link's textCheckingResult should be readonly, initialized at the time the link is added. The rest of the properties, like link attributes, can be manipulated after insertion.
• I'd expect someone to eventually complain about the lack of a userInfo dictionary in the link class. Might be worth weighing that inevitability against the lost dignity of that white flag of surrender.
• Should there be an accompanying removeLink: method?

[getaaron]

Thanks for the thorough feedback.

👍 for combining into one file.

Is there any particular reason that self.links is being renamed to self.linkModels?

The only reason is backwards compatibility. self.links is publicly exposed and historically contains NSTextCheckingResult objects. self.linkModels is private, and the public API (self.links) has remained unchanged:

- (NSArray *) links {
    return [_linkModels valueForKey:@"result"];
}

This could be simplified with breaking changes, but my instinct was to avoid that.

No direct initializer for TTTAttributedLabelLink should be publicly exposed. Instead, links should only ever be initialized as a consequence of methods that add links to the label. Further, addLink... methods should return an instance of the link created by the method, rather than void.

There are two issues with this approach:

1. It disallows subclassing TTTAttributedLabelLink (a link subclass could encapsulate application-specific logic that automatically set style information based on content, for example).
2. It requires the link objects to inform the label when their attributes change. In my current implementation, style information must be set before the link is added, which would be impossible without an initializer for the link objects. This would add additional complexity for handling edge cases (for example, if a developer tries to modify a link's appearance while it's active or inactive.)

These issues are resolvable, but I think would add more complexity to the class than currently exists.

This would be helpful for keeping track of links without an associated text checking result.

Currently, all links have a text checking result. What would a link without a result do?

A link's textCheckingResult should be readonly, initialized at the time the link is added.

👍

The rest of the properties, like link attributes, can be manipulated after insertion.

The links would need to inform the label about this change, which is fine if we decide to move forward with your suggestion to remove the direct initializer for TTTAttributedLabelLink.

I'd expect someone to eventually complain about the lack of a userInfo dictionary in the link class. Might be worth weighing that inevitability against the lost dignity of that white flag of surrender.

We could. Do you mean so other developer can store information there? They could use objc_setAssociatedObject() for that, or subclass the link.

Should there be an accompanying removeLink: method?

I don't think there should be one in this PR, because this PR is intended primarily to fix #331 and the inflexibility of accessibilityValue.

Furthermore, adding removeLink: would be a new feature. It would add two items to the public API (we'd be expected to expose linkModels also). It would also need to handle developers removing links during in all different link states (default, active, inactive). I think developers will rarely need to remove links. Developers that need this can simply set the attributed string again, and then add back only the links they want.

[rsanchezsaez]

I have not yet tried @getaaron changes but looking at the commit and explanations they feel sound.

Very interesting discussion. Looking forward getting these changes merged into the official repo.

Also, consider the possibility of adding block based link handlers, this looks like a good opportunity for that.

[kylef]

@jhersh Anything blocking this PR from being merged?

@getaaron Would you be able to rebase this please.

[jhersh]

I would echo some of Mattt's comments above, in particular:

• Rebase on master please 🔝
• TTTAttributedLabelLink within TTTAttributedLabel.{h,m}
• Adding a text checking result should return the link object(s) created
• Link properties as public -- and readonly -- is a fine first step. Perhaps later we can look at informing the label of link property changes.

For bonus points:

• NSCopying for links
• Tests

[getaaron]

Done:

• Rebase on master please 🔝
• TTTAttributedLabelLink within TTTAttributedLabel.{h,m}
• Adding a text checking result should return the link object(s) created
• NSCopying for links

Not Done:

• Link properties as public -- and readonly

Marking link properties as readonly will make it impossible to do this:

TTTAttributedLabelLink *newLink = [[TTTAttributedLabelLink alloc] init];
newLink.result = // some result…
newLink.activeAttributes = // some attributes…
[label addLink:newLink];

So I didn't move forward with that for now. We could add a custom initializer (initWithResult:attributes:activeAttributes:inactiveAttributes:) with readonly properties, but that will look pretty ugly in practice.

• Tests

No tests… yet.

[getaaron]

Also, we should look at the implementation of addLinksWithTextCheckingResults:attributes: to make sure it works the way we want. As it's written now, the link is initially assigned the label's three attribute dictionaries, overwriting attributes if the developer passed one in. I'm not sure that's desired, although a developer can avoid this behavior using addLink:.

@kylef @jhersh I added you as collaborators on my fork if you want to make any changes. I probably won't be able to get back to this until the weekend.

[jhersh]

(initWithResult:attributes:activeAttributes:inactiveAttributes:) with readonly properties, but that will look pretty ugly in practice.

I'm okay with that. We're gonna need a bigger initializer ⛵

As it's written now, the link is initially assigned the label's three attribute dictionaries, overwriting attributes if the developer passed one in

Wasn't this one of @kylef's concerns in the first place? It'd be good to try and square this one away.

[getaaron]

Wasn't this one of @kylef's concerns in the first place? It'd be good to try and square this one away.

Yes, but without any changes he can work around it now. What should the expected behavior of addLinkWithTextCheckingResult:attributes: be given the related changes in this PR?

• Should activeAttributes and inactiveAttributes be generated somehow, or should they be copied from the label?
• Should the method be updated to require the developer to pass in all three?
• Should we just deprecate addLinkWithTextCheckingResult:attributes: since this functionality can be more simply accomplished by passing a customized link object to addLink:?

[kylef]

@getaaron Quoting the docs, nothing should happen if the attributes there are nil.

The attributes to be added to the text in the range of the specified link. If nil, no attributes are added.

So according to that, I think it should create a link with no active, inactive attributes

[getaaron]

@kylef Just pushed some changes. I tried to minimally change the behavior of addLinkWithTextCheckingResult:attributes: and I documented the new expected behavior. Can you please test my fork against your app and see if it resolves your issue?

[getaaron]

@jhersh Added a little test coverage, including two tests that cover @kylef's issue. Please let me know if there are additional tests you can think of.

[jhersh]

Thanks @getaaron! Would you consider squashing your commits, rebasing, and removing your deletion of the .xcworkspace? It should probably be removed all the same, but that's not related to this pull.

[getaaron]

@jhersh Done.

[jhersh]

I updated some docs and did some minor cleanup. Thanks for this @getaaron!

[kylef]

Nick work, thanks for getting this in @jhersh @getaaron 👍 🍻.