Only one heavy NSDataDetector instance per type (combination)

[gblazex]

NSDataDetector is very heavyweight and it’s unnecessary to create a
separate instance for every single Label. One detector can deal with
a specific type (combination) and so we can just store them in a
dictionary.

This way all Labels using the same data detection type share the same
detector instance.

[gblazex]

With this change TTTLabel will be stacking up quite nicely against UITextView...
UITextView (1.662s) vs TTT (0.084s)

[jhersh]

Very interesting, thanks @galambalazs! It looks like the data detectors are never removed from the dictionary nor freed. Perhaps something worth considering?

[gblazex]

Well if you are using TTTLabel throughout your app, having 1-2-3 data detectors in memory is probably negligible.

This is the whole point. You are not constantly destroying and recreating them.

If you're constantly setting your detection type to All for example, you will have one detector instance for your app. Can't beat that.

[jhersh]

Certainly there is great potential here for allocation savings. I'm speculating about future requests for finer-grained control over data detector lifecycles, and weighing that against greater complexity in a change today. Probably not a major consideration?

[gblazex]

I think YAGNI applies here. No reason to add complexity until it's justified.

(You can certainly make it into a weak NSMapTable and so the Labels would hold the references weakly. That way when there's no label needing it, the detector is freed. It'd basically happen every time you move to a new ViewController screen. But come the next screen, you're gonna end up recreating the data detector(s) eventually.)

[jhersh]

Alas, your performance test above is not measuring what you think it's measuring. The labels and textviews are deallocated in each iteration of your loop well before any data detector work has completed. It might be more insightful to measure the time between allocation and when the data detectors have completed populating links.

[gblazex]

Another test with an actual View Controller (not measureBlock based):

TTT after new Pull Request: https://www.dropbox.com/s/8bdiwhjepxznbsd/ttt.mov?dl=1
UITextView: https://www.dropbox.com/s/rkldfwert98wtly/uitextview_org.mov?dl=1

(testing 1500 UITextViews with short random text, urls, phone numbers. It's all shown on screen in a scrollview, nothing is deallocated).

[gblazex]

It's measuring exactly what I wanted to measure: create Labels with data detectors on them, specifically how the Main thread is affected.

I don't want the data detectors to complete on the main thread. I don't care if they complete at all (as long as they are on the background thread).

It's all about User Experience. That's what I'm testing.

[gblazex]

(as a sidenote TTTLabel data detectors do complete faster as the video shows, but that's not what I'm testing here, strictly label+detector allocation time and main thread impact specifically. That's what this PR improves).

[gblazex]

so what's holding this PR back? :)

[getaaron]

@galambalazs I don't maintain this repo, but some thoughts:

• I like the idea
• Developers should be able to disable this feature
• Developers should be able to invalidate the cache
• Perhaps use NSCache instead of NSDictionary to dispose of the data detectors on memory pressure

Stylistic suggestions:

• I don't think the dictionary should be implemented as a static variable; it should be a separate property
• The functionality should be in a separate method, not embedded in the enabled text checking types setter

Making the two above stylistic suggestions will make it easier to make the feature optional and the cache purgeable. I think this will more or less address @jhersh's suggestions as well.

[gblazex]

Thanks.

You should dispose resources like images taking up megabytes on memory pressure, not NSDataDetectors. They are not memory heavy (just expensive in terms of CPU to create). Especially if you have 1-2 instances total for your app.

There's no need for NSCache because you are not handling large or many instances (the two cases that would justify using anything like that). This only saves memory by sharing them :)

static is what makes sense because it's a shared dictionary.

It can be made optional with a simple if statement.

Still waiting to know what's holding this back.

[harlanhaskins]

Any updates on this? NSFormatter and NSDataDetector instances are expensive.

[gblazex]

been sitting here for 2 months now :-)

[getaaron]

@galambalazs Thanks for your PR, and also your patience. Not sure if you saw, but I'm starting to maintain this project now. I'd like to merge this in essentially as-is, but I have three minor suggestions:

1. I'd like to move this code to the top of the method:

   if (self.enabledTextCheckingTypes == enabledTextCheckingTypes) {
       return;
   }
   
   _enabledTextCheckingTypes = enabledTextCheckingTypes;
   

   This will avoid creating an empty dictionary if enabledTextCheckingTypes is nil and it is set to nil.

2. TTTAttributedLabel supports iOS 4, but NSDictionary subscripting (used in your PR) is only supported in iOS 5 and later. We should switch to the object:forKey: and setObject:forKey: syntax. (I'm open to dropping iOS 4/5 support, but that's another issue.)

3. [NSDataDetector +dataDetectorWithTypes:error:] is failable, and if it fails, this code will crash with "attempt to insert nil value". Although I don't know a situation where it would actually fail, it would be safer to check for success first, rather than immediately sending the result to the dictionary.

Do you have any objections to or concerns with these three changes?

[gblazex]

Awesome. Good to see this project alive.

1. It could be moved to the top. My choice is for readability sake. I like to keep statics at the top of methods and their initialization right after their declaration if possible.
2. Sure error checking can be added. I just pushed this PR fast.

iOS 4

What is holding you back from dropping it? Is there any real-world usage of it at this point? Supporting it seems kind of an academic masturbation to me.

You can just move the current snapshot to a iOS 4.3 legacy branch and whoever wants it can use that.

Master and all new dev branches should follow a reasonable iOS version support (and deprecation policy).

The usefulness of this repo is no longer the backporting of NSAttributedString to labels as that's been available for years in UILabel.

It's async, non-blocking data detection and superior rendering performance compared to the bloated UITextView. When you need more than a UILabel but UITextView would be an overkill.

Feel free to merge this PR and then change as much as you need.

[gblazex]

awesome job @getaaron , kudos for merging!