Skip to content

Let cupertino & material switches move to the right state after dragging - #51606

Merged
fluttergithubbot merged 8 commits into
flutter:masterfrom
LongCatIsLooong:cupertino-switch-fixes
Mar 20, 2020
Merged

Let cupertino & material switches move to the right state after dragging#51606
fluttergithubbot merged 8 commits into
flutter:masterfrom
LongCatIsLooong:cupertino-switch-fixes

Conversation

@LongCatIsLooong

@LongCatIsLooong LongCatIsLooong commented Feb 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request makes Cupertino/Material switches move their thumbs to the correct side after a drag gesture. The onChanged callback is called when the drag gesture ends at a position that indicates the switch should change its value (which is consistent with the onTap behavior, making them both call onChanged when the user reveals the intent to change the value. However this may be a breaking change).

For example, with the below switch:

Switch(
  value: false,
  onChanged: (bool newValue) { },
);

If the thumb is dragged to true (which is still doable), it will animate back to false when the user lets go.

In addition to that, it also moves CupertinoSwitch's gestures/animation logic to its state from the render object.

Related Issues

Fixes #46046

Tests

I added the following tests:

  • "can veto switch dragging result" for both switches.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change. Does not seem to break internal tests.

@fluttergithubbot fluttergithubbot added p: cupertino_ui cupertino_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Feb 28, 2020
@LongCatIsLooong LongCatIsLooong mentioned this pull request Feb 28, 2020
Closed
5 of 13 tasks
@Afsar-Pasha

Afsar-Pasha commented Feb 28, 2020 •
edited
Loading

Copy link
Copy Markdown

The switch bounces a little bit when partially slid
t3
Looks like its caused by changing _position's curves in the middle.
Maybe we can move

_position
  ..curve = Curves.ease
  ..reverseCurve = Curves.ease.flipped;

from _resumePositionAnimation to AnimationStatus when completed or dismissed.

@LongCatIsLooong

LongCatIsLooong commented Feb 29, 2020

Copy link
Copy Markdown
Contributor Author

@Afsar-Pasha thanks for pointing that out! I disabled curve switching for explicit animations (those not triggered by widget.value changes). Turns out there's an issue for a similar problem: #51627.

@Afsar-Pasha

Afsar-Pasha commented Feb 29, 2020

Copy link
Copy Markdown

LGTM
Can you also fix material switch, I tried but it looks like the logic can't be moved from render object to state as _RenderSwitch extends RenderToggleable which contains most of the logic.

Also the current behavior is a little different RenderToggleable(or current CupertinoSwitch) calls onChanged whenever AnimationStatus.completed or AnimationStatus.dismissed(dragging switch to the end will call onChange even without lifting the finger up) but this does not.
I prefer this behavior than the old one, and we can't make material switch to behave like this while extending RenderToggleable.

@LongCatIsLooong

LongCatIsLooong commented Mar 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'll start working on this today.

Should be ready for review now.

@LongCatIsLooong
LongCatIsLooong force-pushed the cupertino-switch-fixes branch 3 times, most recently from c4203f8 to ba3d673 Compare March 16, 2020 20:42
@LongCatIsLooong LongCatIsLooong changed the title Let cupertino switch move to the right state after dragging Let cupertino & material switches move to the right state after dragging Mar 16, 2020
@fluttergithubbot fluttergithubbot added the p: material_ui material_ui package in flutter/packages label Mar 16, 2020
@LongCatIsLooong
LongCatIsLooong force-pushed the cupertino-switch-fixes branch from ba3d673 to f940983 Compare March 16, 2020 21:18
@LongCatIsLooong
LongCatIsLooong marked this pull request as ready for review March 16, 2020 21:18
HansMuller
HansMuller approved these changes Mar 20, 2020
View reviewed changes

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread packages/flutter/lib/src/material/switch.dart
positionController.reverse();
_needsPositionAnimation = true;

if (position.value >= 0.5 != value)

@HansMuller HansMuller Mar 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

Comment thread packages/flutter/lib/src/cupertino/switch.dart Resolved
Comment thread packages/flutter/lib/src/cupertino/switch.dart Outdated

bool get isInteractive => widget.onChanged != null;

// A non-null boolean value that indicates whether the user has stopped dragging

@HansMuller HansMuller Mar 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to true at the end of a drag if the switch must be animated to the position indicated by the widget's value.

Comment thread packages/flutter/lib/src/cupertino/switch.dart Outdated
@override
void didChangeDependencies() {
super.didChangeDependencies();
textDirection = Directionality.of(context);

@HansMuller HansMuller Mar 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think there's any value in caching this value

@fluttergithubbot
fluttergithubbot merged commit 87cbddd into flutter:master Mar 20, 2020
@LongCatIsLooong
LongCatIsLooong deleted the cupertino-switch-fixes branch March 20, 2020 23:18
@Afsar-Pasha Afsar-Pasha mentioned this pull request May 13, 2020
Closed
@pedromassangocode pedromassangocode mentioned this pull request Jul 23, 2020
Open
@goderbauer goderbauer mentioned this pull request Feb 19, 2021
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@HansMuller HansMuller HansMuller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Widget] Unexpected drag behaviour on Switch

5 participants

@LongCatIsLooong @Afsar-Pasha @HansMuller @googlebot @fluttergithubbot