DTActionSheet & DTAlertView working on iOS 6, 7 & 8

[rsanchezsaez]

Hi,

I've seen both pull requests for issues #67 and #68. I think that both present minor problems and I'm offering my alternate solution.

67 has some duplicated code between -init and -initWithTitle:message which makes it difficult to maintain, and breaks the designated initializer convention.

68 is a little bit better because it keeps the core code on init, making it the designated initializer. However, with this solution DTAlertView won't work on iOS 8 if you use iOS' default -initWithTitle:message:delegate:cancelButtonTitle:otherButtonTitles: initializer (as init never gets called in that case).

My solution is an extension and cleanup of #68 by adding code so -initWithTitle:message:delegate:cancelButtonTitle:otherButtonTitles: calls self init, which in turn sets up the _actionsPerIndex blocks dictionary and delegate correctly.

I also fixed DTActionSheet's which presented exactly the same problem.

In addition, I moved the block execution of DTAlertView from -alertView:didDismissWithButtonIndex: to -alertView:clickedButtonAtIndex: which is the correct method to execute the action (same issue as the one you merged already for DTActionSheet).

Finally, I added updated examples of both to the DTProgressHUD demo; improved the DTAlertView unit test to cover the three initializers I mentioned; and added a similar DTActionSheet unit test.

I've successfully tested this code on iOS 6, 7 and 8.

Please note that UIActionSheet and UIAlertView will be deprecated on iOS 8. They are both replaced by UIAlertController (which sadly still doesn't support blocks, lazy Apple!). So it may be a good idea to do a simple block-based DTAlertController class in the future (it should be pretty much the same as the current two). UPDATE: @Cocoanetics kindly pointed that UIAlertController does indeed support blocks. Sorry Apple, you are not lazy now. ;-)

[coveralls]

Coverage increased (+2.52%) when pulling b4d5d99a3db07828bd625d8cec02248da8882cfd on obviousengineering:dtactionsheet into 73604d1a4a5ec7f3516336db4bb7274fa3804a04 on Cocoanetics:develop.

[odrobnik]

@rsanchezsaez please clean up your pull request, there are too many things in there that don't have anything to do with what you're writing.

I cannot evaluate your fix like this.

UIAlertController does have block-support via UIAlertAction

[odrobnik]

PS: By "cleanup" I mean, branch off the latest develop, cherry-pick specific commits and send a pull request for this new branch.

[rsanchezsaez]

Everything in my pull request has to do with what I'm writing.

The problem is that some of the work is split over too many commits, because midway through I realized a mistake and couldn't rollback because I had already pushed to my online repo. On top of that I had to undo merge #67. If I cherry-pick it will be incomplete: my only option for cleanup would be manually redoing the work again in order to produce better ordered and more compact commits. I don't think it's worth the effort, the resulting code would be exactly the same.

If you look at the simple diff between the tip of my branch and the tip of your develop you'll see that the changes are only what I mentioned above and that they are very easy to see and understand.

[odrobnik]

@rsanchezsaez at first glance it looked like you branched off an earlier version.

Now why is it that people always send me great pull requests exactly a day too late to make it into a release? That always forces me to re-tag a release...

[rsanchezsaez]

Hmm, I originally branched from an earlier version but merged deveop into my branch before doing this. Maybe that's what got you confused. ;-)

Sorry about the bad timing! I know, retagging kind of sucks (it could leave people with the pre-retag version, because pod update doesn't notice retags...).

[odrobnik]

@rsanchezsaez And you are 100% certain, that all is tested and working on iOS 6-8?

[rsanchezsaez]

iOS 7 and 8 100%. Let me check again on iOS 6 to be extra sure.

[odrobnik]

ok, waiting for your report. Will go to sleep now and probably merge/re-tag it in the morning.

[odrobnik]

PS: One more thing that bothers me.... why would you call the init the "designated initializer"... this is title is usually reserved for the constructor that is supposed to be used. Who in his right mind would initialize an alert view or action sheet with init only? You would always use an initWithTitle... method, thus this should be the designated initializer IMHO. What do you think?

[rsanchezsaez]

Compiling on Xcode 5.1.1 (5B1008) with Base SDK iOS 7.1 and Deployment Target iOS 6.0 seems to work completely fine on the iOS 6.1 simulator. I don't have any actual device with iOS 6.x to test further.

[rsanchezsaez]

A designated initializer is not the initializer "you are supposed to call". A designated initializer is the one that leaves the object in the minimum valid operating state. All other secondary initializers at the same class level should first call the designated initializer at the same class level, and then perform extra optional customization.

The idea is that there cannot exist initializers for a given subclass that leave the object in an invalid state (like it was the case with -initWithTitle:message:delegate:cancelButtonTitle:otherButtonTitles:). At the same time, there should not be two initializers that, independently from each other, initialize the state of the object (as was the case with fix #67, which was creating _actionsPerIndex in two different places). This would mean that we have two designated initializers, which makes subclassing very hard.

These "agreed rules" (they are not enforced by the compiler, they are just a convention which facilitates subclassing and usage of the subclasses) result in that each subclass has to have one (and only one) designated initializer (which is the one that you would call using [super init...] from a sub-subclass). They can also have a number of optional secondary initializers that call [self designatedInitializer] (and never [super init...] directly) which initialize the object to a valid state, and then perform additional "convenience customization".

The user can freely use whichever initializer is more convenient for them, without having to worry about which one is the designated one, because they all will work. This designated initializer stuff is basically guidelines for people doing subclasses.

In the case of my fix, you can readily use a DTActionSheet or DTAlertView after just using plain init. After init the objects have correct state, will work fine, and you can customize all their properties and buttons (using the message and title properties, the addButton... methods, etc.) to achieve the same state as a secondary initializer would get to.

UIActionSheet and UIAlertView work the same way: if you create them through init they'll just work fine. (But Apple doesn't explicitly state this in the documentation, because they say that these two classes should not be subclassed --ha!--. I guess they changed behaviour on iOS 8 because of this "subclasses forbidden" policy. I think that the documentation implies that the designated initializer is -initWithTitle:message:delegate:cancelButtonTitle:otherButtonTitles: but that was not the case on iOS 6/7: the designated one was init, that's why the old DT... inits worked well. I guess that Apple switched the designated init in their UI... classes to the long one on iOS 8, so that's why it no longer calls [self init] at the UI... level. And thus they broke the old DT... implementation. In order to maintain backwards compatibility, we have to always call [super init] from DT... which is the designated on iOS 6/7 but the secondary on iOS 8. So this is not the completely correct way of subclassing, but it works in all cases).

There's a more detailed explanation with some nice diagrams at this Twitter Dev blog post.

This rule has some exceptions: e.g., some UIView's have two designated initializers: initWithFrame: (for programmatically creating views) and initWithCoder: (which gets called when views are created through Storyboard or .XIB files). But this is the exception, not the norm.

[odrobnik]

thank you for your big contribution and explanation!

[odrobnik]

PS: I included this in retagged 1.7.2 release.

[rsanchezsaez]

You're welcome. I'm happy to contribute. :-)