Skip to content

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

Merged
fluttergithubbot merged 8 commits intoflutter: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 intoflutter:masterfrom
LongCatIsLooong:cupertino-switch-fixes

Conversation

@LongCatIsLooong
Copy link
Copy Markdown
Contributor

@LongCatIsLooong LongCatIsLooong commented Feb 28, 2020

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 f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Feb 28, 2020
@Afsar-Pasha
Copy link
Copy Markdown

Afsar-Pasha commented Feb 28, 2020

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
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
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
Copy link
Copy Markdown
Contributor Author

LongCatIsLooong commented Mar 13, 2020

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 f: material design flutter/packages/flutter/material repository. 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
Copy link
Copy Markdown
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

positionController.reverse();
_needsPositionAnimation = true;

if (position.value >= 0.5 != value)
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 Outdated

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

// A non-null boolean value that indicates whether the user has stopped dragging
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.

@override
void didChangeDependencies() {
super.didChangeDependencies();
textDirection = Directionality.of(context);
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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Widget] Unexpected drag behaviour on Switch

5 participants