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

Move source of truth for duration and repeat style to the execution phase #43

Closed
NickEntin opened this issue Sep 14, 2020 · 2 comments · Fixed by #49
Closed

Move source of truth for duration and repeat style to the execution phase #43

NickEntin opened this issue Sep 14, 2020 · 2 comments · Fixed by #49
Milestone

Comments

@NickEntin
Copy link
Collaborator

Stagehand separates the animation process into two phases: construction and execution. The construction phase consists of building up an Animation. The execution phase begins with a call to Animation.perform(...), optionally followed by calls to the returned AnimationInstance to control the animation.

Setting the animation's duration and repeat style are currently considered to be part of the construction phase and are controlled by the Animation.duration and Animation.repeatStyle properties, respectively.

This has some interesting characteristics when it comes to composing animations. An animation's duration and repeat style is effectively meaningless when it's added as a child to another animation. This can cause some confusion, since consumers may expect the parent animation to take its children's values into account.

This also affects the reusability of common animation components, even if they aren't being used as part of a larger animation. For example, consumers could define an animation factory that vends a common fadeOutAnimation that is used whenever they need to fade a view out. It is quite likely that the same duration and repeat style should not be used for all instances of a reusable animation across the app, so each use case may need to set these properties before executing the animation.

// Most construction happens in the factory.
var animation = AnimationFactory.fadeOut

// Do some final construction to get it ready.
animation.duration = 5
animation.repeatStyle = .repeating(count: 2, autoreversing: true)

// Execute the animation.
animation.perform(on: view)

Note that this code snippet could be cleaned up by injecting the duration and repeat style into the factory; however, this has the same root issue in that these values are being defined by the code responsible for the execution of a particular instance of this animation, rather than the construction of the common animation.

To solve this, we can move the source of truth for an animation's duration and repeat style to the execution phase. Specifically, we can add duration and repeatStyle parameters to the Animation.perform(...) method.

// Construction happens in the factory.
let animation = AnimationFactory.fadeOut

// Execute the animation with the desired duration and repeat style.
animation.perform(
    on: view,
    duration: 5,
    repeatStyle: .repeating(count: 2, autoreversing: true)
)

A similar change would also be made to the AnimationQueue.enqueue(...) method for that execution path.

animationQueue.enqueue(
    animation: animation,
    duration: 5,
    repeatStyle: .repeating(count: 2, autoreversing: true)
)

There are also cases, however, where consumers want to promote consistency in usage of a given animation. This is currently well-supported by the duration and repeatStyle properties on Animation. To continue supporting this use case, instead of removing these properties, we can rename them to defaultDuration and defaultRepeatStyle. The duration and repeatStyle parameters of the Animation.perform(...) method will then be made optional, where a value of nil indicates that the animation's default should be used.

// Construct the animation with a default duration and repeat style.
animation.defaultDuration = 5
animation.defaultRepeatStyle = .none

// Execute the animation using the default duration and repeat style.
animation.perform(on: view)

// Execute the animation with a duration of 2 and the default repeat style.
animation.perform(on: view, duration: 2)

The same changes will be reflected on AnimationGroup as well: updating the current AnimationGroup.duration and AnimationGroup.repeatStyle properties to the equivalent default-prefixed versions and adding duration and repeatStyle parameters to the AnimationGroup.perform(...) method.

Alternatives

This would be a breaking change in the framework. Alternatively, we could leave the Animation.duration and Animation.repeatStyle properties as-is, while still introducing the parameters in Animation.perform(...) to allow overriding the values in the execution phase. This would not be a breaking change, since unchanged code would use the default values. I think the breaking version of this is better for the framework in the long term, however, since using the default-prefixed property names makes it clearer that these do not represent the source of truth for the duration of the final animation instance.

Related Changes

Alongside this change, we may also want to update AnimationRepeatStyle.none to .noRepeat. This avoids a warning that appears when using .none with an optional AnimationRepeatStyle:

Assuming you mean 'Optional<AnimationRepeatStyle>.none'; did you mean 'AnimationRepeatStyle.none' instead?
@NickEntin
Copy link
Collaborator Author

This change is partially motivated by the introduction of interactive animations (see #34). Getting the separation of the phases (construction and execution) correct will be increasingly important as we introduce different mechanisms for the execution (like interactive animations) that may not support all of the same functionality as the existing execution mechanisms (non-interactive execution and animation queues).

@NickEntin
Copy link
Collaborator Author

After chatting with @seanho and @lukebradford about this, I think implictDuration and implictRepeatStyle are clearer than using "default" as the prefix. We can then differentiate in documentation between the implicit duration that's set in the construction phase (Animation.implicitDuration) and the explicit duration that's optionally set in the execution phase (e.g. via perform(on:duration:)).

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 a pull request may close this issue.

1 participant