Respect the attributedText property's 'copy' attribute.

[jparise]

attributedText is defined as a copy accessor, so we need to -copy the NSAttributedString's value in -setAttributeText:.

[AliSoftware]

Thanks for your pull request. You're right there is some inconsistency between the @propertydeclaration and the implementation.

But I believe I was doing some copy before and I changed it to some standard retain for a reason, after multiple refactorings and optimisations. So I believe that the right fix is the other way around, namely changing the @property(copy) to @property(retain) !

In earlier versions I modified this _attributedText to add automatically detected links to it, that's why I did a copy. But normally in the latest versions I keep a separate copy of the NSAttributedString with links added to it so that it does not modify the original one. This way if we change the automaticallyAddLinksForTypes property value, we can recompute the links on the original string. So in that case the MRC_RETAINis right, but that's the @property(copy) that should be @property(retain) instead.

Will check all that probably next weekend (don't have time right now), just to be sure to apply the right fix.

Thx

[jparise]

Yes, earlier code was doing copy. That's how I noticed this change when I upgraded. :smile:

I think copy is the correct behavior for this type of property. My reasoning:

We generally build attributed strings using NSMutableAttributedStrings and then assign them to the label. The expectation should be that the caller no longer needs to worry about maintaining that original mutable string instance after that point. They may want to reuse that string instance for another label after making a few modifications, for example.

Second, copy gives Foundation the opportunity to transform the original NSMutableAttributedString instance into an immutable NSAttributedString. The latter may have lower memory overhead. To be fair, I don't know if it does this, but it leaves the optional on the table. This is of course one of the reason why some objects have both copy and mutableCopy methods.

And third, UILabel (in iOS 6) has an attributedString property that is defined as a copy accessor, so it's helpful to model OHAttributedLabel on Apple's API here.

Does that reasoning make sense to you?

[AliSoftware]

Yes that reasoning makes sense, and now that you says it I think I remembered keeping the (copy) attributed because iOS6's UILabel itself has its attributedString property set as (copy). As OHAttributedLabel does inherit from UILabel and I use the same name for the property, I am forced to keep the same attributes for this property; otherwise the compiler will complain about the inconsistency when compiling with the iOS6 SDK.

So as a matter of fact, I can't change the property attribute from copy to retain anyway, and will need to apply your suggestion!

I think I kept the MRC_RETAIN here to avoid the memory and performance overhead of copying the whole string, because it can become an issue when using an UITableViews in which all your cells have one or more OHAttributedLabels: in that case when you scroll your table view quickly, given the additional time taken to perform a copy of the NSAttributedString each time you assign it to your recycled cells's OHAttributedLabels, it may introduce a performance issue and make your scroll to be less fluid. But you're right, I really need to make the behavior match the property copy attribute and copy it anyway, no matter what. (And hope Cocoa implements the CopyOnWrite principle on this one to avoid unnecessary copy if the original is never modified to improve performance)

---

PS: About building an immutable NSAttributedString from the NSMutableAttributedString, in practice I am quite sure that Apple did implement both NSAttributedString and NSMutableAttributedString with the same CoreFoundation underlying object, which has all the functions to mutate the string, and some flag to tell the runtime if the string is mutable or not. At least that's how NSMutableArray, NSMutableDictionary and NSMutableString are implemented, AFAIK.

You can test this by calling some mutable method (like appendAttributedString for example) on an NSAttributedString which should be immutable (of course you will have a warning but ignore it for this test). When running this code, your app will crash by an exception like "tried to mutate an immutable instance", and not the "unrecognized selector sent to instance" exception, meaning that the method exists but detect it has been called on an instance marked as immutable.