Add support for removing added animations #42
Conversation
|
Gentle ping. |
|
Slightly less gentle ping @randallli @romoore @jgunaratne |
src/private/MDMAnimationRegistrar.h
Outdated
| #import <Foundation/Foundation.h> | ||
| #import <QuartzCore/QuartzCore.h> | ||
|
|
||
| // Tracks animations that have been added to a layer. |
There was a problem hiding this comment.
Nit: Would be nice to have a documentation for the interface and methods even though it's a private class.
src/private/MDMAnimationRegistrar.m
Outdated
| #import "MDMRegisteredAnimation.h" | ||
|
|
||
| @implementation MDMAnimationRegistrar { | ||
| NSMapTable *_layersToRegisteredAnimation; // Map of [CALayer:MDMRegisteredAnimation] |
There was a problem hiding this comment.
Nit: Use lightweight generics?
NSMapTable<CALayer *, MDMRegisteredAnimation*> *_layersToRegisteredAnimation;| #pragma mark - Private | ||
|
|
||
| - (void)forEachAnimation:(void (^)(CALayer *, CABasicAnimation *, NSString *))work { | ||
| // Copy the registered animations before iteration in case further modifications happen to the |
There was a problem hiding this comment.
Looking at this comment, are you worried that the dictionary can be mutated while you are iterating over it? Are the work blocks the source of mutation?
There was a problem hiding this comment.
Yep - consider if we remove an animation, its associated completion block might get fired and a new animation might be registered as a result.
| } | ||
|
|
||
| - (BOOL)isEqual:(id)object { | ||
| return [_key isEqual:object]; |
There was a problem hiding this comment.
Two registered animations are equal if they have the same key but different animations? Could you please add some basic unit tests for this class?
There was a problem hiding this comment.
We don't have a convention in place for private API unit tests unfortunately, meaning we can't access the APIs from unit test targets without some change to both our podfile & bazel configs.
| XCTAssertEqual(view.layer.opacity, 0.5) | ||
| } | ||
|
|
||
| #if swift(>=3.2) |
There was a problem hiding this comment.
Is it the XCTWaiter that requires swift 3.2? The "classic" approach is to run the main runloop - does that not work here?
This change introduces two new APIs:
removeAllAnimationsstopAllAnimationsThe difference between the two APIs is that one simply removes the animations, while the other commits the presentation layer value to the model layer value before removing the animation. The latter is primarily intended for use when implementing motion with gesture recognition.