-
Notifications
You must be signed in to change notification settings - Fork 29.7k
CupertinoSlidingSegmentedControl update
#152976
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
CupertinoSlidingSegmentedControl update
#152976
Conversation
ce2986d to
7af6ff4
Compare
CupertinoSlidingSegmentedControl update
8efda17 to
3465fb2
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| Alignment scaleAlignment; |
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.
final Alignment scaleAlignment;
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.
There's a fourth state where both isLeft and isRight are true that seems unhandled?
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.
Sorry not sure I understand this correctly, how to be both left and right segment? For the segment that isLeft, it is the leftmost segment in the segmented control, and for isRight, it is the rightmost segment (assuming text direction is ltr).
| bool enabled = true; | ||
| if (widget.setEnabled != null && widget.setEnabled!.containsKey(entry.key)) { | ||
| enabled = widget.setEnabled![entry.key]!; | ||
| } |
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.
final bool enabled = widget.setEnabled?.[entry.key] ?? true.
Also you can use a set to store disabled segments.
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.
Updated!
| .merge(TextStyle(fontWeight: widget.highlighted ? FontWeight.w500 : FontWeight.normal)), | ||
| .merge(TextStyle( | ||
| fontWeight: widget.highlighted ? FontWeight.w600 : FontWeight.w500, | ||
| fontSize: 13, |
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.
Use constants for hardcoded values and document the provenance?
| // The minimum opacity of an unselected segment, when the user presses on the | ||
| // segment and it starts to fadeout. | ||
| // | ||
| // Inspected from iOS 13.2 simulator. |
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.
consider documenting the source of this const: #21302
| ); | ||
|
|
||
| // The height of the separator. | ||
| const double _kSeparatorHeight = 18.0; |
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.
nit: If both the height and the width are const, turn them into a Size const?
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.
There are several places that width and height are used separately, maybe just keep them as is?
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.
I didn't realize those are only used directly by the widgets / render objects. They are used to construct other constants it looks like?
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.
I'm a bit confused by _kSeparatorHeight. It looks like it's not used directly but used to compute _kSeparatorInset. So the real meaning of _kSeparatorHeight is "the height of the separator when the segmented control is at the min height"?
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 you are right. I was thinking providing a const height value might be more straightforward to understand. But it seems only "inset" doesn't change, when segments height have some larger values, the separator height is also getting larger. Updated.
| final List<RenderBox> children = getChildrenAsList(); | ||
|
|
||
| // Children contains both segment and separator and the order is segment -> | ||
| // separator -> segment. So to paint separators, indes should start from 1 and |
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.
typo: index?
| currentThumbRect = unscaledThumbRect; | ||
|
|
||
| double delta = 0; | ||
| if (isLeadingChild) { |
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.
what if it's both leading and trailing?
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 the same explanation as above:) This should mean leftmost and rightmost ones
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.
Oh I didn't realize assert(children.length >= 2). Nvm.
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.
But in that case they're logically mutually exclusive. Maybe use an enum to model that instead of 2 bools.
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.
Just pushed a commit, does this look okay? 51277b6
| } | ||
|
|
||
| final int? highlightedChildIndex = highlightedIndex; | ||
| final bool isLeadingChild = highlightedChildIndex != 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.
uber nit: the null check is not needed?
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.
It would be possible that groupValue is null, and we don't have highlighted child index, right?
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.
if highlightedChildIndex == 0 then it can't be 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.
That's right! Thanks for catching this! Updated.
QuncCccccc
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.
Thanks so much for the review! I fixed all the comments except the setEnabled-related comments. Will push separate commits for these.
| ); | ||
|
|
||
| // The height of the separator. | ||
| const double _kSeparatorHeight = 18.0; |
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.
There are several places that width and height are used separately, maybe just keep them as is?
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| Alignment scaleAlignment; |
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.
Sorry not sure I understand this correctly, how to be both left and right segment? For the segment that isLeft, it is the leftmost segment in the segmented control, and for isRight, it is the rightmost segment (assuming text direction is ltr).
| child: widget.child, | ||
| ), | ||
| ), | ||
| widget.child, |
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.
I tired to keep it but seems only removing both Offstage and DefaultTextStyle can solve the shaking issue. Keeping DefaultTextStyle still make the overall size shakes a little during thumb switch.
| } | ||
|
|
||
| final int? highlightedChildIndex = highlightedIndex; | ||
| final bool isLeadingChild = highlightedChildIndex != 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.
It would be possible that groupValue is null, and we don't have highlighted child index, right?
| currentThumbRect = unscaledThumbRect; | ||
|
|
||
| double delta = 0; | ||
| if (isLeadingChild) { |
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 the same explanation as above:) This should mean leftmost and rightmost ones
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
51277b6 to
5a59efc
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
LongCatIsLooong
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.
Adding an enabled state for segments introduces a corner case where a segment is disabled (by rebuild) during a drag gesture, and it looks like it's not handled in the implementation? Could you add tests for that corner case?
Also what's the expected behavior of onValueChanged, if the current selection is disabled?
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
| /// | ||
| /// Disabled children cannot be highlighted and don't show any animation | ||
| /// when they are tapped or long pressed. So if this set contains [groupValue], | ||
| /// there is no segment getting highlighted until it is switched to an enabled |
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 that mean the original selected segment, if any, is unselected?
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.
packages/flutter/lib/src/cupertino/sliding_segmented_control.dart
Outdated
Show resolved
Hide resolved
| final HorizontalDragGestureRecognizer drag = HorizontalDragGestureRecognizer(); | ||
| final LongPressGestureRecognizer longPress = LongPressGestureRecognizer(); | ||
|
|
||
| T? get groupValue => widget.disabledChildren.contains(widget.groupValue) ? null : widget.groupValue; |
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.
nit: Well this works but usually this will become a problem in the didUpdateWidget in the implementation becasue now you can't compare groupValue from the old widget (if that's need that is).
| if (highlighted == newValue) { | ||
| return; | ||
| } | ||
| // If `disabledChildren` set contains the `groupValue`, then no update |
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.
What happens if a drag is started, and highlighted becomes disabled and the gesture lands in highlighed? (Consider adding a test case).
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 I think I include the demo in the PR description. When we are dragging the thumb to the disabled segment, the thumb will stop and still highlight the previous one. Does this make sense? Will create one test!
Screen.Recording.2024-08-12.at.6.27.11.PM.mov
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.
I meant that the parent widget rebuilds so the currently selected segment becomes disabled. See also: #152976 (review)
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.
Thanks so much for all the review suggestions! Added unit tests for both highlight behavior and onValueChanged behavior. If the highlighted is added to disabledChildren during drag, then when dragging is completed, the highlight will disappear, but onValueChanged will still be called. The onValueChanged behavior is simulated from XCode example.
| final T segment = segmentForXPosition(details.localPosition.dx); | ||
| onPressedChangedByGesture(null); | ||
| if (segment != widget.groupValue) { | ||
| if (segment != groupValue) { |
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.
nit: combine the nested ifs using &&?
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.
Or do you need this enable check? groupValue is null if disabled no?
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
d257a02 to
5b64f18
Compare
LongCatIsLooong
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.
IMO it makes more sense to allow selecting a disabled segment programmatically. It works better with the onValueChanged callback and it gives developers full control over the behavior. You can select a disabled segment in UISegmentedControl programmatically too.
| child: DefaultTextStyle.merge( | ||
| style: const TextStyle(fontWeight: FontWeight.w500), | ||
| child: widget.child, | ||
| DefaultTextStyle.merge( |
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.
nit: consider adding a comment for why the indexed stack and the widget replica.
| /// there is no segment getting highlighted until it is switched to an enabled | ||
| /// segment. | ||
| /// | ||
| /// [onValueChanged] will not be called if the segment is disabled. However, if |
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.
The [onValueChanged] callback will not be called if the currently selected segment becomes disabled,
| /// | ||
| /// [onValueChanged] will not be called if the segment is disabled. However, if | ||
| /// an enabled segment is "highlighted" by dragging gesture and becomes disabled | ||
| /// before dragging stops, [onValueChanged] will be triggered when release finger. |
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.
This sounds a bit confusing. What will be selected?
5b64f18 to
4e982a8
Compare
Roll Flutter from 2e221e7 to 303f222 (77 revisions) flutter/flutter@2e221e7...303f222 2024-09-12 [email protected] Manual roll to 48ddaf578fb0c8326d5b4b680b0f49ea72e33216 (flutter/flutter#155070) 2024-09-12 [email protected] Externalize and update onboarding instructions (flutter/flutter#154730) 2024-09-12 [email protected] when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected (flutter/flutter#155049) 2024-09-12 98614782+auto-submit[bot]@users.noreply.github.com Reverts "iOS: update provisioning profile for 2024-2025 cert (#155052)" (flutter/flutter#155059) 2024-09-12 [email protected] iOS: update provisioning profile for 2024-2025 cert (flutter/flutter#155052) 2024-09-11 [email protected] Factor out `Container` objects (flutter/flutter#153619) 2024-09-11 [email protected] Move (`dev/tools`), complete v0 of `native_driver` (Android) (flutter/flutter#154843) 2024-09-11 [email protected] Roll Flutter Engine from ade8ef293bc6 to ee5adf6d2ee1 (2 revisions) (flutter/flutter#155046) 2024-09-11 [email protected] Fix `flutter run` on Mac x64 hosts if Swift Package Manager is enabled (flutter/flutter#154645) 2024-09-11 [email protected] Roll Packages from bb53e5d to 4c18648 (1 revision) (flutter/flutter#155033) 2024-09-11 [email protected] Roll Flutter Engine from 4eb729b7a5c4 to ade8ef293bc6 (3 revisions) (flutter/flutter#155031) 2024-09-11 [email protected] fix: Dropdown menu trying to access highlight element which doesn't exist when search and filters both are enabled (flutter/flutter#151969) 2024-09-11 [email protected] Marks Linux build_tests_3_5 to be unflaky (flutter/flutter#154993) 2024-09-11 [email protected] Add 'direction' allow to 'SegmentedButton' oriented vertically (flutter/flutter#150903) 2024-09-11 [email protected] Marks Linux build_tests_5_5 to be unflaky (flutter/flutter#154995) 2024-09-11 [email protected] Update the signature of DDS launcher callback. (flutter/flutter#154949) 2024-09-11 [email protected] Migrate Color.toString() test, improves `equalsIgnoringHashCodes` (flutter/flutter#154934) 2024-09-11 [email protected] Update material and cupertino localizations (flutter/flutter#154959) 2024-09-11 [email protected] Marks Linux build_tests_1_5 to be unflaky (flutter/flutter#154991) 2024-09-11 [email protected] Marks Linux build_tests_2_5 to be unflaky (flutter/flutter#154992) 2024-09-11 [email protected] Fix `flutter create` warning regarding Java compatibility (flutter/flutter#152836) 2024-09-11 [email protected] Roll Flutter Engine from 54757dab9462 to 4eb729b7a5c4 (1 revision) (flutter/flutter#155022) 2024-09-11 [email protected] Fix java version used by `build_aar_module_test` (flutter/flutter#154967) 2024-09-11 [email protected] Roll Flutter Engine from 0a14c519ea4f to 54757dab9462 (1 revision) (flutter/flutter#155015) 2024-09-11 [email protected] Roll Flutter Engine from 35a3171b72c5 to 0a14c519ea4f (1 revision) (flutter/flutter#154984) 2024-09-11 [email protected] Roll Flutter Engine from b9c0b96c3316 to 35a3171b72c5 (1 revision) (flutter/flutter#154980) 2024-09-11 [email protected] Roll Flutter Engine from 52eeea075767 to b9c0b96c3316 (1 revision) (flutter/flutter#154976) 2024-09-11 [email protected] Roll Flutter Engine from a26075f9b1e6 to 52eeea075767 (1 revision) (flutter/flutter#154973) 2024-09-11 [email protected] Roll Flutter Engine from 60c15bc0f40e to a26075f9b1e6 (6 revisions) (flutter/flutter#154969) 2024-09-11 [email protected] Migrate `apple-mobile-web-*` to `mobile-web-*`. (flutter/flutter#154964) 2024-09-11 [email protected] Roll Flutter Engine from 8a038a6f7099 to 60c15bc0f40e (15 revisions) (flutter/flutter#154960) 2024-09-10 [email protected] Adds dart fixes for Color opacity functions (flutter/flutter#154953) 2024-09-10 [email protected] Missing benchmarks for `foundation/all_elements_bench.dart` (flutter/flutter#154954) 2024-09-10 [email protected] Update color assertions (flutter/flutter#154752) 2024-09-10 [email protected] handle EAGAIN (macOS) in ErrorHandlingProcessManager (flutter/flutter#154306) 2024-09-10 [email protected] fix unpack freezing app with animation duration zero (flutter/flutter#153890) 2024-09-10 [email protected] Remove last `--disable-dart-dev` in `flutter/flutter`. (flutter/flutter#154948) 2024-09-10 [email protected] Remove scheduler: luci from new `build_aar_module_test` (flutter/flutter#154945) 2024-09-10 [email protected] Roll pub packages (flutter/flutter#154939) 2024-09-10 [email protected] `CupertinoSlidingSegmentedControl` update (flutter/flutter#152976) 2024-09-10 [email protected] Roll pub packages (flutter/flutter#154933) 2024-09-10 [email protected] fix test `chrome.close can recover if getTab throws a StateError` (flutter/flutter#154889) 2024-09-10 [email protected] SearchBar context menu (flutter/flutter#154833) 2024-09-10 [email protected] Fix `flutter build aar` for modules that use a plugin (flutter/flutter#154757) 2024-09-10 [email protected] Roll Packages from b4e0fc1 to bb53e5d (4 revisions) (flutter/flutter#154926) 2024-09-10 [email protected] Clean up `SnackBar` inherit theme data test (flutter/flutter#154921) ...

This PR is to improve
CupertinoSlidingSegmentedControlfidelity and add a new propertysetEnabledso that segments can be disabled now.Fidelity update includes:
Alignment.leftCenterandAlignment.rightCenterrespectively, assuming the text direction is left to right. For segments in middle, they should have center alignment as previously.Alignment update demo
Screen.Recording.2024-08-08.at.1.33.19.PM.mov
Comparison before and after
Before:

After:

Shaking issue
Before:
Screen.Recording.2024-08-08.at.1.47.53.PM.mov
After:
Screen.Recording.2024-08-08.at.1.47.35.PM.mov
The
disabledChildrenfeature can be used to disable segments. The default disabled color comes from the inspection in the Xcode example.Disabled feature demos
Disabled segment cannot be highlighted:
Screen.Recording.2024-08-09.at.6.03.40.PM.mov
groupValuecan still be highlighted programmatically if it is disabled.Pre-launch Checklist
///).