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

Added disableAnimator method so we can disable constraint animator proxy... #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iosdev-republicofapps
Copy link

... when no longer needed.

We need to be able to disable the animator proxy when it's no longer needed. This is necessary because we may have a sequence like:

constraint 1 - make animated changes
constraint 1 - make non-animated changes
constraint 1 - make animated changes

So we need to be able to turn off the animator proxy as needed. As the animator property is currently implemented on MASConstraint, it permanently enables the animator. We need to be able to disable the animator on a given constraint object without having to destroy/rebuild/remove/remake that constraint.

All this PR does is reverse the enabling of the animator proxy on a constraint. To use the proxy again, the caller just needs to use the animator property again on the constraint and it will be re-enabled.

The cost of doing this is infinitesimally small: all we're doing is setting and unsetting a single boolean.

This is currently a blocking bug for me so the sooner we could get this merged into a release that would be ideal. :-) I am very happy to help in any way that I can - please don't hesitate to ask.

All tests pass after this change.

@cloudkite
Copy link
Contributor

Good point, I hadn't thought of that

How about If we created a MASAnimatorConstraint which wraps a MASConstraint

this means that any calls to [MASConstraint animator] would return this new class

- (MASConstraint *)animator {
    return [[MASAnimatorConstraint alloc] initWithConstraint:self];
}

Thus meaning the following would work

self.someConstraint.animator.offset = 10 //animated change
self.someConstraint.offset = 200 //non animated
self.someConstraint.animator.offset = -200 //animated

@cloudkite
Copy link
Contributor

closing due to inactivity

@cloudkite cloudkite closed this Oct 19, 2015
@iosdev-republicofapps
Copy link
Author

Sorry for the inactivity. Could we re-open this now so I can work on this again? :-)

I'm curious why you'd prefer to wrap the MASConstraint?

I'd have a preference for not wrapping it because I don't want to have to create a new constraint, store a reference to it, destroy the new constraint and then restore the old reference every time I need to animate a constraint. That feels bug-prone and costly to me.

I'm open to input though, so let me know what you think please. Do you object to turning off the animator proxy? It seemed like the natural complement to turning it on.

Please let me know. I'm happy to help. And thanks for creating and maintaining Masonry - I really appreciate you doing this even though more attention might be on SnapKit in Swift.

@cloudkite
Copy link
Contributor

cloudkite commented Jun 29, 2016

@iosdev-republicofapps

I think for consistency animator should be single use just like it is in the Cocoa APIs. current behaviour is quite confusing as it sets and forgets the useAnimator variable.

ie

self.someconstraint.animator.offset = 100;
// useAnimator is set to true.
// meaning all future changes are animated
// even if we make a change without the animator property

// its weird that will now be animated because of the previous call 
// ie you shouldnt have to call [self.someconstraint disableAnimator]
self.someconstraint.offset = 200;

I think using composition instead of a useAnimator variable is much more elegant as it leaves the original constraint unchanged and therefore removes the weird side effect of setting useAnimator permanently to true, that using .animator causes.

@cloudkite cloudkite reopened this Jun 29, 2016
@iosdev-republicofapps
Copy link
Author

@cloudkite thanks for re-opening. Let me take a look at composition and get back to you ...

@cloudkite
Copy link
Contributor

cloudkite commented Jun 29, 2016

@iosdev-republicofapps I guess another way of approaching this could be to
change setLayoutConstant in MASViewConstraint to the following

- (void)setLayoutConstant:(CGFloat)layoutConstant {
    _layoutConstant = layoutConstant;

#if TARGET_OS_MAC && !(TARGET_OS_IPHONE || TARGET_OS_TV)
    if (self.useAnimator) {
        self.useAnimator = NO;
        [self.layoutConstraint.animator setConstant:layoutConstant];
    } else {
        self.layoutConstraint.constant = layoutConstant;
    }
#else
    self.layoutConstraint.constant = layoutConstant;
#endif
}

This is a much simpler change than composition. However it doesn't completely get rid of the side effect but it's alot less complicated so might be good enough.

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.

2 participants