Allow a single view to be automatically distributed

[pnc]

NSArray's autoDistributeViewsAlongAxis: requires 2 views, but the code works perfectly with just a single subview! Since the NSAssert is from the initial commit, I thought perhaps this had been an early sanity check and not a specific design decision.

I think it might make more sense to only require a single view (or perhaps none at all, in which case the method might no-op, monad-style) so that callers don't have to check the number of items in the array and implement different functionality in the case of a single view.

I'd be happy to add a test for the new single-view case if you're open to this patch.

One use case is a view with a small but variable number of children, such as a toolbar of buttons.

[coveralls]

Coverage remained the same at 73.7% when pulling afe0b8a on eleostech:no-minimum into 0a0b452 on smileyborg:master.

[smileyborg]

Interesting, the use case you gave does make sense. I buy the idea of making these methods work with 1, and even 0 views (no-op). I didn't consider these cases when originally implementing these methods, so are you sure that they work correctly in the 1-view case for all possible parameters? I wouldn't be surprised if something broke for one or more parameters in the single view case. This would definitely be a good thing to extend the unit tests to cover.

Related, some of the other methods in the NSArray category also validate that there are at least 2 views. Doing a quick pass it seems that most of these would not make sense with just 1 view. Do you think the asserts make sense for those, or would you expect them to just be no-ops for the 1 or 0 view cases?

I guess something else to consider is whether the asserts may be more useful in catching a real programmer error, or whether supporting the use cases like the one you mentioned is more important.

[smileyborg]

@pnc Any thoughts on my comments above? This would be a good change to get in sooner rather than later, before the v3.0.0 work (#69) really kicks off.

[pnc]

@smileyborg Sorry for the delay! I was working on tests for this and ended up having to give some OSS love to some of my own projects.

I think we should leave the asserts for those that wouldn't be able to do anything with fewer views, since those asserts aren't blocking any functionality and the programmer can just not call them if there aren't enough views. For consistency, I think it also makes sense to drop the assert to > 0 for these methods rather than removing it entirely. Perhaps a rule of thumb could be that if there are too few views to create any constraints at all, the method should fail the assertion? I'll need to look through the full list of methods later to figure out which ones this holds for.

For 3.0.0, I'd absolutely make an argument for all array methods to be no-ops if there are no views or there are too few views for the constraint to be applied. I'm a big fan of fail-fast asserts, but I think the benefits of a safe method you can call without conditional logic fit Objective-C a little better.

[smileyborg]

OK, take a look at the other methods when you get a chance and let me know. But I think I agree with the assert for 0 views, because that seems unlikely to be a common use case and more likely to be an error of some sort (e.g. array was unexpectedly empty). So basically, the change will be to make the asserts >= 1 view.

[pnc]

@smileyborg Okay, updated!

I didn't find any other methods for which it really made sense to lower the required number of views. The autoDistributeViewsAlongAxis… methods are the only ones that deal with the views' superview, and can thus do something useful with just a single view in the array.

To that end, the only thing I had to change to get the existing implementations to work with a single view was a tweak to how al_commonSuperviewOfViews handles the single-view case. Although there was a test case checking that the "common superview" of a single view was that view itself, this isn't consistent with how al_commonSuperviewOfViews is used—the only call site (in the autoDistributeViewsAlongAxis…withFixedSize implementation) expects it to always return a containing superview of the views in the list, which it now does.

Let me know if you'd like me to incorporate this work into the 3.0 branch instead.

[pnc]

I can make a separate PR to remove the assertions for all these array methods for 3.0. I think doing so would allow library users to write less branchy layout code.

[smileyborg]

Makes sense to me. I will get this merged in tomorrow or this weekend for sure.

As far as the upcoming 3.0 release, that work is being done on master; in other words, the next release will be 3.0. So all of these changes are perfectly fine in this PR!

And regarding this:

I can make a separate PR to remove the assertions for all these array methods for 3.0. I think doing so would allow library users to write less branchy layout code.

Are you referring to making these distribute methods just no-ops when called with 0 views?

[pnc]

@smileyborg

Are you referring to making these distribute methods just no-ops when called with 0 views?

Yep! Sorry to be unclear. I didn't want to pack it onto this PR so you could merge this and then make a separate decision about that change. If you're on board, I can add those changes directly to this one.

[smileyborg]

OK, reviewed the changes and looks great! Just committed them to master. Thanks again for taking the time to submit this!

Now, just in case you're interested, regarding this:

To that end, the only thing I had to change to get the existing implementations to work with a single view was a tweak to how al_commonSuperviewOfViews handles the single-view case. Although there was a test case checking that the "common superview" of a single view was that view itself, this isn't consistent with how al_commonSuperviewOfViews is used—the only call site (in the autoDistributeViewsAlongAxis…withFixedSize implementation) expects it to always return a containing superview of the views in the list, which it now does.

The reason you had to make that slight change in behavior to al_commonSuperviewOfViews is because the original goal of that method was to return the appropriate view where we could add the necessary constraints. And if you only have 1 view involved in a constraint, you are allowed to add that constraints to itself. Now, this method isn't being used for this purpose anymore -- it's just used for distributing views -- so the new behavior actually makes more sense. (Moreover, we no longer need to identify the right view to add constraints to; we just activate and deactivate them and let Apple deal with it.) Just in case you were wondering about this.

Finally, regarding allowing the 0 view use case, I've been thinking about it and actually changed my mind here. The 1-view case is definitely great to support, but I think the 0-view case is a little murkier, especially in Objective-C where it's very easy to have nil sneak up on you. The real-world 0-view use case seems relatively unlikely, especially compared to the likelihood of some sort of bug causing your view array to be empty when you didn't want or expect that. Given this, I think keeping in the debug assert is beneficial in order to hopefully help catch a real issue early on, instead of "silently failing". (And technically, even the current code should support 0 views without crashing in a Release build when asserts are compiled out.) Do you agree?