CupertinoSlidingSegmentedControl #42775

LongCatIsLooong · 2019-10-15T17:43:08Z

Description

Introduce CupertinoSlidingSegmentedControl, the iOS 13 CupertinoSegmentedControl.

Related Issues

Closes #33796

Tests

add packages/flutter/test/cupertino/sliding_segmented_control_test.dart

TODOs

momentary is missing: https://developer.apple.com/documentation/uikit/uisegmentedcontrol/1618586-ismomentary
Currently only supports equalWidth layout. proportional is missing.
The thumb shadow is done exactly as the sketch file describes, but it looks a bit off when compared with the native component.
The drag gesture recognizer has to be passed to the render object because it needs to know children's dimensions. We probably need a CupertinoMultiChildGestureDetector or something, as this kind of gesture is quite common on iOS.

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

No, this is not a breaking change.

…ntrol

justinmc

Thanks for tackling this!

There are a few questions in my comments below that I wanted to clear up, and a few nitpicks, otherwise looks good.

Does this support dark mode by the way?

justinmc · 2019-10-15T17:58:20Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

Could there be any problem here with HashMap being unordered? See https://master-api.flutter.dev/flutter/dart-core/Map-class.html. LinkedHashMap is ordered.

Actually I copied these from CupertinoSegmentedControl 😅 . The child widgets will be visually placed in the same order as they appear in the Map, if the Map is unordered then the children may appear in a different order every time the widget rebuilds.

Maybe add a disclaimer like I see elsewhere: The [children] argument must be an ordered [Map] such as a [LinkedHashMap]. I feel like it will be easier for people to end up with the order changing on them without understanding why.

justinmc · 2019-10-15T18:02:58Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

This seems to be incomplete.

justinmc · 2019-10-15T18:05:24Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

Why is this a stateful widget with the children defined as a variable even though they never change?

justinmc · 2019-10-15T18:10:04Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

Just for my own curiosity going forward, what is our plan for parameters like this versus theming?

I remember I had the same question and I vaguely remember Hans' answer was themes are complicated so we want to make a careful decision. @HansMuller did I just put words in your mouth?

The CupertinoTheme is pretty spare and I think that's a good thing. Providing widget parameters for a few visual customizations is also a good thing. Our primary goal is to look native by default. Enabling the developer to make sweeping changes to component types with the CupertinoTheme isn't really a goal.

justinmc · 2019-10-15T18:13:00Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

I mentioned in another comment up above that there might be a concern about ordered vs. unordered Maps. Maybe just say the same thing up there as this says here, if needed.

You mean to mention such as a [LinkedHashMap]?

Yeah the part about The [children] argument must be an ordered [Map] such as a [LinkedHashMap].. I'm responding on that comment above (#42775 (comment)).

justinmc · 2019-10-15T20:22:18Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+/// See also:
+///
+///  * <https://developer.apple.com/design/human-interface-guidelines/ios/controls/segmented-controls/>
+class CupertinoSlidingSegmentedControl<T> extends StatefulWidget {


What's the reason for keeping the old CupertinoSegmentedControl around and making this a new widget? Can you use both on native?

Actually though, I see that someone is requesting to still be able to use the old one: #33796 (comment)

Assuming that we are keeping both, is there any code that can be shared between the two, or are there tests that could cover both? I didn't look very closely, but just checking that you have tried to avoid code duplication as much as possible.

The reason for keeping both is the API of CupertinoSlidingSegmentedControl is drastically different from CupertinoSegmentedControl. It's probably too big of a breaking change (and breaks every existing CupertinoSegmentedControl) to introduce in one go. Introducing it as a new widget will allow people to migrate as they feel comfortable. But yeah we will have to maintain both widgets with duplicated code for the time being now.

Ah I see, makes sense. Is it worth adding a @deprecated flag to CupertinoSegmentedControl now?

IMO for now it doesn't hurt for them to coexist, as the comment you mentioned requested.

justinmc · 2019-10-15T20:44:12Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+  }
+
+  // Play highlight animation for the child located at _highlightControllers[at].
+  void animateHighlightController({ T at, bool forward }) {


I think this and maybe a few other methods could be private.

I guess it doesn't make much difference for instances of state, but just something I noticed.

justinmc · 2019-10-15T20:55:17Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+//    The other is the separator fadein/fadeout animation.
+//
+// 4. A tap down event on the segment pointed to by the current selected
+//    index will trigger a CABasciaAnimation that shrinks the thumb to 95% of its


Typo, "CABasciaAnimation" => "CABasicAnimation"

justinmc · 2019-10-15T20:55:52Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+//    original size, even if the sliding animation is still playing. The
+///   corresponding tap up event inverts the process (eyeballed).
+//
+// 5. A tap down event on other segments will trigger a CABasciaAnimation


Typo on CABasciaAnimation again.

justinmc · 2019-10-15T21:18:39Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+
+      await tester.tap(find.byIcon(const IconData(1)));
+      await tester.pump();
+      await tester.pump(const Duration(milliseconds: 500));


Does pumpAndSettle not work?

njovy · 2019-10-16T17:53:43Z

 @override
  Widget build(BuildContext context) {
    super.build(context);
    return CupertinoPageScaffold(
      child: CustomScrollView(slivers: <Widget>[
        CupertinoSliverNavigationBar(
          largeTitle: Text("Test"),
          middle: CupertinoSlidingSegmentedControl<int>(
            children: {
              0: Padding(
                padding: EdgeInsets.symmetric(horizontal: 24, vertical: 6),
                child: Text(
                  "Test1",
                  style: TextStyle(fontSize: 14),
                ),
              ),
              1: Padding(
                padding: EdgeInsets.symmetric(horizontal: 24, vertical: 6),
                child: Text("Test2", style: TextStyle(fontSize: 14)),
              ),
              2: Padding(
                padding: EdgeInsets.symmetric(horizontal: 24, vertical: 6),
                child: Text("Test3", style: TextStyle(fontSize: 14)),
              ),
            },
            controller: ValueNotifier<int>(0),
          ),
        ),
        SliverFillRemaining(
          child: Container(),
        )
      ]),
    );
  }

onTap is not called if I use this widget with a Sliver widget inside CustomScrollView
Above is a sample code that scrolling works but tapping.
Thank you for this rapid update!

justinmc

LGTM assuming @njovy's use case works.

Sorry if I spam you with mismatched comments and reviews, I'm failing at using Github right now...

LongCatIsLooong · 2019-10-18T01:28:11Z

@njovy Thank you so much for trying out the PR and catching the bug! I've updated the gesture handling so it works (seemingly) correctly in conjunction with a VerticalDragGestureRecognizer now.

@justinmc thanks for the review! Updated per your comments, I hope I didn't miss anything. (Turns out I did miss the dark mode comment. Yes it has dark mode support).

njovy · 2019-10-18T08:34:31Z

@LongCatIsLooong It works like a charm with the latest commit. Thank you so much!

HansMuller · 2019-10-21T16:28:32Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+
+// The spring animation used when the thumb changes its rect.
+final SpringSimulation _kThumbSpringAnimationSimulation = SpringSimulation(
+  const SpringDescription(mass: 1, stiffness: 503.551, damping: 44.8799),


The stiffness and damping values seem oddly specific. What are they based one?

I got those values from the native UISegmentedControl.

HansMuller · 2019-10-21T16:31:25Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+/// must be an ordered [Map] such as a [LinkedHashMap], the ordering of the keys
+/// will determine the order of the widgets in the segmented control.
+///
+/// When the state of the segmented control changes, the widget changes [controller]'s


[controller]'s => the [controller]'s

HansMuller · 2019-10-21T16:33:35Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+/// control cease to be selected.
+///
+/// A segmented control can feature any [Widget] as one of the values in its
+/// [Map] of [children]. The type T is the type of the keys used to identify each


The use of "keys" here might confuse developers who think we're referring to Key (it confused me). Even though the point is immediately clarified in the following text. Maybe say "[Map] keys" and just "map keys" after that.

Also: it would be helpful to explain, at the outset, that the map's key are segmented control's values.

HansMuller · 2019-10-21T16:36:54Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+///   2: Text('Child 3'),
+/// };
+///
+/// final ValueNotifier<int> controller = ValueNotifier<int>(null);


Might include a comment here like:

// null means no selection

HansMuller · 2019-10-21T16:43:33Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

The CupertinoTheme is pretty spare and I think that's a good thing. Providing widget parameters for a few visual customizations is also a good thing. Our primary goal is to look native by default. Enabling the developer to make sweeping changes to component types with the CupertinoTheme isn't really a goal.

HansMuller · 2019-10-21T18:19:40Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+  @override
+  double computeMinIntrinsicWidth(double height) {
+    RenderBox child = firstChild;
+    double minWidth = 0.0;


Really maxMinChildWidth or widestMinChildWidth

HansMuller · 2019-10-21T18:21:34Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+    double minWidth = 0.0;
+    while (child != null) {
+      final _SegmentedControlContainerBoxParentData childParentData = child.parentData;
+      final double childWidth = child.getMinIntrinsicWidth(height) + 2 * _kSegmentMinPadding;


Could just factor the _kSegmentMinPadding in at the end? Like:

return childCount * (widestMinChildWidth + 2 * _kSegmentMinPadding) + totalSeparatorWidth;

HansMuller · 2019-10-21T18:22:09Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+      maxWidth = math.max(maxWidth, childWidth);
+      child = childParentData.nextSibling;
+    }
+    return maxWidth * childCount + totalSeparatorWidth;


Similar comments about var name and etc as above.

HansMuller · 2019-10-21T18:29:53Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+
+  @override
+  double computeMinIntrinsicHeight(double width) {
+    RenderBox child = firstChild;


Could this be:

return getChildrenAsList.fold<double>(0, (double minHeight, RenderBox child) { return math.max(minHeight, child.getMinIntrinsicHeight(width)); });

Here and for computeMaxIntrinsicHeight().

getChildrenAsList's documentation says

/// This function is useful when you need random-access to the children of
/// this render object. If you're accessing the children in order, consider
/// walking the child list directly.

I think the imperative way is slightly more efficient?

I don't think the efficiency aspect will matter, but there's no harm in leaving this code as it is. It's pretty easy to read.

HansMuller · 2019-10-21T18:31:27Z

packages/flutter/lib/src/cupertino/sliding_segmented_control.dart

+    if (highlightedIndex != null) {
+      if (_childAnimations == null) {
+        _childAnimations = <RenderBox, _ChildAnimationManifest> { };
+        for (int i = 0; i < childCount - 1; i++) {


Please use i += 1, here and elsewhere.

…ntrol

HansMuller

LGTM

HansMuller · 2019-10-29T23:56:19Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+      expect(e.toString(), contains('children'));
+    }
+
+    final Map<int, Widget> children = <int, Widget>{};


FWIW you could write:

const Map<int, Widget> children = <int, Widget>{1: Text('Child 1'), 2: Text('Child 2')};

Here and below.

HansMuller · 2019-10-29T23:58:51Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+    Future<void> verifyPadding({ EdgeInsets padding }) async {
+      final EdgeInsets effectivePadding = padding ?? const EdgeInsets.symmetric(vertical: 2, horizontal: 3);
+      final Rect segmentedControlRect = tester.getRect(find.byKey(key));
+


Extra blank line

HansMuller · 2019-10-30T00:02:29Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+
+    final Map<int, Widget> children = <int, Widget>{};
+    children[0] = const SizedBox(
+      height: double.infinity,


Maybe include a comment about using height=infinity to ensure that the child consumes all of the available vertical space, like SizedBox.expand() does for both dimensions.

turns out I didn't need the SizedBox since I'm checking the Rect of the Opacity widget

HansMuller · 2019-10-30T00:07:26Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+    (WidgetTester tester) async {
+      final Map<int, Widget> children = <int, Widget>{};
+      children[0] = const Text('Child 1');
+      children[1] = Container(


children[1] = SizedBox(width: 50, height: 50);

HansMuller · 2019-10-30T00:11:23Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+      find.byKey(const ValueKey<String>('Segmented Control')),
+    );
+
+    expect(segmentedControl.size.width.isFinite, isTrue);


Couldn't we check the actual width of the segmented control here? Could simplify the expected width arithmetic by making the children SizedBox(width: 100) or something similar.

HansMuller · 2019-10-30T00:14:54Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+
+    // Vertical drag works for the scroll view.
+    final TestGesture gesture = await tester.startGesture(tester.getCenter(find.text('Child 1')));
+    // The first moveBy doesn't actually move the scrollable. It's there to make


Thanks for the explanation

HansMuller · 2019-10-30T00:16:20Z

packages/flutter/test/cupertino/sliding_segmented_control_test.dart

+  testWidgets('Insert segment while animation is running', (WidgetTester tester) async {
+    final Map<int, Widget> children = SplayTreeMap<int, Widget>(
+      (int a, int b) => a - b,
+      (dynamic key) => true,


Is this parameter needed?

LongCatIsLooong added 30 commits September 15, 2019 21:59

remove CupertinoSystemColors

5429acf

wrong color

ef62e88

update docs

3863235

comments

f0ccec2

update comments

2492004

update tests

25d0469

s/gray/grey/

a805a8f

Merge branch 'colors' into segmented-control

1ea8e41

WIP

6d9fbef

WIP

0031cc4

WIP

122595e

Merge remote-tracking branch 'upstream/master' into segmented-control

b38361e

update

73dfb09

shadow

b59050c

update

9211272

update

a7e70ef

Merge remote-tracking branch 'upstream/master' into segmented-control

8e19be7

update

da80a10

Pog

64fd92d

use simulation instead of curve

4fe7c79

create new class for segmeneted control

25230ee

update

6417102

Merge branch 'master' of github.com:flutter/flutter into segmented-co…

f298565

…ntrol

update

61f2074

Merge remote-tracking branch 'upstream/master' into segmented-control

12af914

update

8c8088f

update

edc2a4b

Merge branch 'master' of github.com:flutter/flutter into segmented-co…

ea23445

…ntrol

WIP

f6ac023

Merge remote-tracking branch 'upstream/master' into segmented-control

d9c738c

interactive example

ec9e6f4

LongCatIsLooong force-pushed the segmented-control branch from 38c79cf to ec9e6f4 Compare October 15, 2019 21:17

justinmc reviewed Oct 15, 2019

View reviewed changes

update comments

5f31a14

Merge remote-tracking branch 'upstream/master' into segmented-control

6a02f56

LongCatIsLooong changed the title ~~CupertinoSlidingSegmentedControl~~ [WIP] CupertinoSlidingSegmentedControl Oct 17, 2019

justinmc approved these changes Oct 17, 2019

View reviewed changes

update gesture handling

924e27a

LongCatIsLooong changed the title ~~[WIP] CupertinoSlidingSegmentedControl~~ CupertinoSlidingSegmentedControl Oct 18, 2019

LongCatIsLooong force-pushed the segmented-control branch from 439eb28 to f82338d Compare October 18, 2019 01:25

make stuff private

e4bdd9f

LongCatIsLooong force-pushed the segmented-control branch from f82338d to e4bdd9f Compare October 18, 2019 01:32

HansMuller mentioned this pull request Oct 18, 2019

Upcoming Cupertino Library Features #43041

Closed

18 tasks

HansMuller reviewed Oct 21, 2019

View reviewed changes

LongCatIsLooong added 7 commits October 21, 2019 14:50

Merge branch 'master' of github.com:flutter/flutter into segmented-co…

72c80ed

…ntrol

Review

69782a4

AnimatedBuilder

90e0077

more tests

3088eaf

Merge branch 'master' of github.com:flutter/flutter into segmented-co…

1daeaf3

…ntrol

Merge remote-tracking branch 'upstream/master' into segmented-control

10790a8

update assertion message

ea92272

HansMuller approved these changes Oct 30, 2019

View reviewed changes

review

11743a0

LongCatIsLooong merged commit 9ea0ecc into flutter:master Oct 30, 2019

LongCatIsLooong deleted the segmented-control branch October 30, 2019 17:42

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

CupertinoSlidingSegmentedControl (flutter#42775)

5e14552

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

CupertinoSlidingSegmentedControl #42775

CupertinoSlidingSegmentedControl #42775

Uh oh!

Conversation

LongCatIsLooong commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

TODOs

Checklist

Breaking Change

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njovy commented Oct 16, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njovy commented Oct 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Oct 15, 2019 •

edited

Loading

LongCatIsLooong commented Oct 18, 2019 •

edited

Loading