Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested batches (without combining their constraints) #52

Merged
merged 1 commit into from Dec 19, 2017

Conversation

sanekgusev
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@ZevEisenberg ZevEisenberg self-requested a review December 13, 2017 15:20
@sanekgusev
Copy link
Contributor Author

@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
Copy link
Collaborator

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 jvisenti merged commit 3576864 into Rightpoint:master Dec 19, 2017
@jvisenti
Copy link
Contributor

Released as 4.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants