Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Jun 9, 2024

After adding a toggle method, I realized that there really aren't any notable use cases for the from parameter; instead, it made sense to have an ability to toggle based on a boolean value other than isForwardOrCompleted.

This pull request updates the toggle method accordingly and implements it in the relevant places.


Example (motion_demo_fade_scale_transition.dart):

// before
ElevatedButton(
  onPressed: () {
    if (_isAnimationRunningForwardsOrComplete) {
      _controller.reverse();
    } else {
      _controller.forward();
    }
  },
  // ...
)


// after
ElevatedButton(
  onPressed: _controller.toggle,
  // ...
)

And I think my favorite example is from the aptly-named toggleable.dart:

// before
if (tristate) {
  if (value == null) {
    _positionController.value = 0.0;
  }
  if (value ?? true) {
    _positionController.forward();
  } else {
    _positionController.reverse();
  }
} else {
  if (value ?? false) {
    _positionController.forward();
  } else {
    _positionController.reverse();
  }
}


// after
if (tristate && value == null) {
  _positionController.value = 0.0;
}
_positionController.toggle(forward: value ?? tristate);

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Jun 9, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review June 9, 2024 17:25
@nate-thegrate nate-thegrate requested a review from goderbauer June 10, 2024 16:24
///
/// 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.
Copy link
Member

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?

Copy link
Contributor Author

@nate-thegrate nate-thegrate Jun 10, 2024

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()?

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!

Copy link
Member

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.

Copy link
Contributor Author

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.

@nate-thegrate nate-thegrate force-pushed the toggle branch 2 times, most recently from ad7ffb8 to efefa65 Compare June 10, 2024 23:43
Copy link
Contributor Author

@nate-thegrate nate-thegrate left a 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

Comment on lines 528 to 534
TickerFuture toggle([ bool? newDirection ]) {
return _toggle('toggle', newDirection ?? !isForwardOrCompleted, null);
}
Copy link
Contributor Author

@nate-thegrate nate-thegrate Jun 13, 2024

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!)

@gnprice
Copy link
Member

gnprice commented Jun 13, 2024

@nate-thegrate You might find the description of the avoid_positional_boolean_parameters lint rule helpful in expanding on the point @goderbauer was making above about the API:
https://dart.dev/tools/linter-rules/avoid_positional_boolean_parameters

I think an API like controller.toggle(forward: true) would probably go a long way to address that concern.

@Piinks
Copy link
Contributor

Piinks commented Jun 26, 2024

Greetings from triage! What is the status of this PR?

@nate-thegrate
Copy link
Contributor Author

Thanks for the reminder—totally got sidetracked with other PRs!

I believe it should be ready for review at this point.

@nate-thegrate nate-thegrate requested a review from goderbauer June 26, 2024 18:25
auto-submit bot pushed a commit that referenced this pull request Jul 2, 2024
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).
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
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 nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jul 12, 2024
@justinmc
Copy link
Contributor

justinmc commented Aug 1, 2024

@nate-thegrate Heads up that this has some merge conflicts.

@nate-thegrate
Copy link
Contributor Author

@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 from parameter instead of just replacing it, is that right?

@nate-thegrate
Copy link
Contributor Author

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.

@justinmc
Copy link
Contributor

justinmc commented Aug 1, 2024

Sounds good, I agree that would be easier to land 👍

@nate-thegrate nate-thegrate deleted the toggle branch August 1, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: animation Animation APIs a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. refactor Improving readability/efficiency without behavioral changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants