Rightpoint / Anchorage

A collection of operators and utilities that simplify iOS layout code.
MIT License
627 stars 46 forks source link

Support nested batches (without combining their constraints) #52

Closed sanekgusev closed 6 years ago

sanekgusev commented 6 years ago

I have a feeling that you guys may have already considered this, but I thought I'd suggest this nested constraint batch implementation. IDK how this fits in conceptually, so treat this as a proposal.

Right now Anchorage does not support nested batches at all and trying to create one while another batch is already in place is considered programming error. However, after I started using Anchorage extensively, I've found that this gets in the way every now and then, with higher-level code suddenly having to care/know about whether lower-level code is using batches to set up its constraints or not. At the same time, a naïve nested batch implementation is trivial to implement. Which is what this PR is about.

Instead of keeping a global reference to a single (optional) active batch, I've expanded the concept to store a stack of active batches in an array, with newly created constraints being recorded into the most recent batch in the global “stack”. Now, this implementation keeps all the nested batches completely isolated from each other (i. e. constraints added to any of the nested batches would not appear and would not be affected by the active flag of the batches higher up in the batch stack). It feels reasonably intuitive to me, but I guess others might feel different.

Thoughts?

jvisenti commented 6 years ago

We had discussed this when first implementing batching, but I forget what the rationale was for not supporting nesting. Probably just that it added complexity, and we didn't have a clear enough use case for it at the time. I'm fine with adding this feature as it is, but also want @ZevEisenberg's input because he worked on batching originally.

ZevEisenberg commented 6 years ago

I believe we originally did just a single batch for simplicity. But you raise a good point about outer code needing to worry about inner code, and vice versa. I think that makes a good case for merging this (in theory - I haven't had a chance to review the code itself).

ZevEisenberg commented 6 years ago

Oh, and I also like the use case outlined here, where you use nested batching to control activation:

let constraints = Anchorage.batch {
    view1.widthAnchor == view2.widthAnchor
    nestedConstraints = Anchorage.batch(active: false) {
        view1.heightAnchor == view2.heightAnchor / 2 ~ .low
    }
    view1.leadingAnchor == view2.leadingAnchor
}

…although I think you'd want to assign anything inside the (active: false) branch to a variable in practice; otherwise, you can't easily retrieve that constraint later.

sanekgusev commented 6 years ago

@ZevEisenberg I actually find that I only use batching with active: false, for those constraints that might change later, so that I can store all of the batch's constraints into some [NSLayoutConstraint] ivar (that ivar's willSet and didSet observers take care of deactivating old constraints and activating newly set ones, respectively). I can then later assign another batch of inactive constraints to the same ivar, and replace the previous ones.

For those constraints that will not change throughout the lifetime of an instance, I barely find a need to batch them — I just set them up one after another.

ZevEisenberg commented 6 years ago

I’ve used batching with active: true before. I have an app where the UI is modeled by a state machine. Every time the state changes, I deactivate all constraints and then build new ones from scratch, so I use batching there for cleanliness and ostensibly performance reasons (it’s documented as being faster to batch-activate constraints).

jvisenti commented 6 years ago

Released as 4.2.1