-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Implementing AnimationController.toggle()
#149966
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
Conversation
| /// | ||
| /// If [from] is non-null, it will be set as the current [value] before running | ||
| /// the animation. | ||
| /// If [toggleForward] is non-null, it determines the animation direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that controller.toggle(true) is the same as calling controller.forward() and controller.toggle(false) is the same as controller.reverse()?
From an API perspective I find it difficult to wrap my head around this API. It is not super-clear what the boolean means. Why is true == forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that
controller.toggle(true)is the same as callingcontroller.forward()andcontroller.toggle(false)is the same ascontroller.reverse()?
Yes!
Technically, we could get rid of .forward() and .reverse() and have everything implemented in terms of .animateTo() and .animateFrom() with relatively minimal refactoring. But I like having "forward" and "reverse" since they're common use cases.
Likewise, I think it makes sense to have .toggle(), since the following AnimationController logic is very common:
if (condition) {
_controller.forward();
} else {
_controller.reverse();
}From an API perspective I find it difficult to wrap my head around this API. It is not super-clear what the boolean means. Why is true == forward?
This is something I hadn't considered. After looking it over again, it does feel redundant to include the method in the parameter name, and I agree that it should have a better explanation.
I've made updates to the method accordingly; thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem with the API is more that when I see controller.toggle(true) I don't actually immediately know what that means. Toggle doesn't naturally take a boolean, so it makes the code harder to understand compared to the if/else pattern, which is immediately obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, that makes sense: fewer lines of code isn't necessarily worth it if it becomes significantly more difficult to understand.
I 100% agree that toggle(true) and toggle(false) is extremely unintuitive—animated_cross_fade.dart is an egregious example of this:
_controller.toggle(switch (widget.crossFadeState) {
CrossFadeState.showFirst => false,
CrossFadeState.showSecond => true,
});
// much more readable as a switch statement
switch (widget.crossFadeState) {
case CrossFadeState.showFirst:
_controller.reverse();
case CrossFadeState.showSecond:
_controller.forward();
}However, I believe that the vast majority of the toggle calls in this PR are very intuitive & easy to follow, for example:
@override
void didUpdateWidget(_RallyTab oldWidget) {
super.didUpdateWidget(oldWidget);
_controller.toggle(widget.isExpanded);
}My immediate thought upon reading: "This looks like a stateful widget with an animation controller that toggles each time isExpanded changes."
If I were to give a letter grade to the current additions, it would probably be as follows:
A+ for the empty .toggle() calls (6 in this PR)
A-: "simple" arguments (e.g. widget.isExpanded, 21 in this PR)
_controller.toggle(_showDrawerContents);
_controller.toggle(shouldOpenList);
_codeBackgroundColorController.toggle(
_demoStateIndex.value == _DemoState.code.index,
);
_controller.toggle(widget.isExpanded);
_positionController.toggle(widget.value);
_controller.toggle(widget.mode == DatePickerMode.year);
enableController.toggle(widget.isEnabled);
selectController.toggle(widget.selected);
deleteDrawerController.toggle(hasDeleteButton);
_opacityController.toggle(widget.visible);
_orientationController.toggle(_orientationController.isDismissed);
_controller.toggle(widget.isExpanded);
_hoverColorController.toggle(widget.isHovering);
_controller.toggle(hasError);
_controller.toggle(widget.isSelected);
_extendedController.toggle(widget.extended);
enableController.toggle(isEnabled);
_state.enableController.toggle(isInteractive);
_controller.toggle(widget.isOpen);
_reactionFocusFadeController.toggle(focused);
_reactionHoverFadeController.toggle(hovering);B for calls with a boolean or null-aware operator (7 examples)
_controller.toggle(widget.visibility?.value ?? true);
_controller.toggle(widget.visibility?.value ?? true);
_positionController.toggle(value ?? tristate);
avatarDrawerController.toggle(hasAvatar || widget.selected);
_controller.toggle(!widget.checked);
menuAnimation.toggle(!menuAnimation.isCompleted);
_floatingLabelController.toggle(
_floatingLabelEnabled && widget._labelShouldWithdraw,
);C for the calls containing multiple operators (2 examples)
_state.overlayController.toggle(hovered && (hoveringStartThumb || hoveringEndThumb));
_moveController!.toggle(
_moveController!.value > (widget.dismissThresholds[_dismissDirection] ?? _kDismissThreshold),
);F for animated_cross_fade.dart
_controller.toggle(switch (widget.crossFadeState) {
CrossFadeState.showFirst => false,
CrossFadeState.showSecond => true,
});I'll go ahead and revert those 3 worst examples.
ad7ffb8 to
efefa65
Compare
nate-thegrate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it'd be nice to finish up this PR soon, regardless of whether or not toggle() remains as it is currently.
I'd be happy with any of the following:
- merging the pull request as-is
- simplifying the
toggle()method by removing the parameter - removing the
toggle()method
| TickerFuture toggle([ bool? newDirection ]) { | ||
| return _toggle('toggle', newDirection ?? !isForwardOrCompleted, null); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Toggles the direction of this animation.
///
/// The value of `forward` should toggle between `true` and `false`, and
/// `toggle()` should be called each time it changes. This function acts
/// like `forward()` or `reverse()` based on whether the value passed is
/// `true` or `false`, respectively.
///
/// If no argument is provided, the controller changes direction every time,
/// based on the current [status].
///
/// {@macro flutter.animation.AnimationController.ticker_canceled}
TickerFuture toggle({ bool? forward }) {
return _toggle('toggle', forward ?? !isForwardOrCompleted, null);
}Here's the current method documentation (edited based on @gnprice's comment, thanks!)
|
@nate-thegrate You might find the description of the I think an API like |
|
Greetings from triage! What is the status of this PR? |
|
Thanks for the reminder—totally got sidetracked with other PRs! I believe it should be ready for review at this point. |
While I was working on #149966, I noticed a couple of ways that `_DismissibleState` could be tweaked to be easier to follow. Changes are described in comments below, and we're currently [awaiting a test exemption](https://discord.com/channels/608014603317936148/608018585025118217/1251255349277626490).
While I was working on flutter#149966, I noticed a couple of ways that `_DismissibleState` could be tweaked to be easier to follow. Changes are described in comments below, and we're currently [awaiting a test exemption](https://discord.com/channels/608014603317936148/608018585025118217/1251255349277626490).
|
@nate-thegrate Heads up that this has some merge conflicts. |
|
@justinmc thanks! My understanding is that the next stable release happens in 2 weeks, and if this isn't merged by then, we'll need to deprecate the |
|
After thinking it over, it'd probably be best to have a PR that just focuses on the parameter change, rather than mixing it with a bunch of refactoring. |
|
Sounds good, I agree that would be easier to land 👍 |
After adding a
togglemethod, I realized that there really aren't any notable use cases for thefromparameter; instead, it made sense to have an ability to toggle based on a boolean value other thanisForwardOrCompleted.This pull request updates the
togglemethod accordingly and implements it in the relevant places.Example (motion_demo_fade_scale_transition.dart):
And I think my favorite example is from the aptly-named toggleable.dart: