CupertinoDynamicColor and friends by LongCatIsLooong · Pull Request #37719 · flutter/flutter

LongCatIsLooong · 2019-08-06T20:53:20Z

Description

Introduce CupertinoDynamicColor
Introduce CupertinoUserInterfaceLevel
Introduce CupertinoSystemColors

Related Issues

#35541

Tests

I added the following tests:

packages/flutter/test/cupertino/colors_test.dart

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.

HansMuller

It would be OK to create cupertino_dynamic_color.dart and cupertino_system_colors.dart. Separate files help with reading code, since there's no need to worry about private dependencies.

HansMuller · 2019-08-12T23:50:40Z

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

@@ -1,8 +1,21 @@
-// Copyright 2017 The Chromium Authors. All rights reserved.
+// Copyright 2019 The Chromium Authors. All rights reserved.


This is an existing file; the header shouldn't change.

HansMuller · 2019-08-12T23:55:40Z

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

+    @required Color darkElevatedColor,
+    @required Color highContrastElevatedColor,
+    @required Color darkHighContrastElevatedColor,
+  }) : this._(


Best to assert that all of the color parameters are non-null; if the CupertinoDynamicColor._ constructor fails the error message is likely to be confusing.

If I remember correctly if you're calling a constructor from the same class (i.e. not super.foo) you won't be able to add asserts or any other expressions.

oh never mind.

Never mind the never mind... although the static analyzer didn't give me trouble, when I tried to run flutter test it failed to compile:

src/cupertino/colors.dart:148:3: Error: Final field '_colorMap' is not initialized by this constructor.
Try to initialize the field using an initializing formal or a field initializer.
CupertinoDynamicColor({
^
lib/src/cupertino/colors.dart:321:33: Context: '_colorMap' is defined here.
final List<List<List>> _colorMap;
^^^^^^^^^

HansMuller · 2019-08-12T23:56:37Z

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

+  CupertinoDynamicColor._(
+    Color value,
+    this._colorMap,
+  ) : assert(() {


If the CupertinoDyanmicColor asserts that all of its parameters are non-null, this shouldn't be needed.

HansMuller · 2019-08-13T00:00:55Z

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

+  /// or a [MediaQuery] whose [MediaQueryData.platformBrightness] is [PlatformBrightness.light].
+  /// - has a [MediaQuery] whose [MediaQueryData.highContrast] is `false`.
+  /// - has a [CupertinoUserInterfaceLevel] that indicates [CupertinoUserInterfaceLevelData.base].
+  Color get color => _colorMap[1][0][0];


I like seeing the 3 bits like this :-).

HansMuller · 2019-08-13T00:03:35Z

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

+/// color in the color family.
+///
+/// When used as a regular color, `CupertinoDynamicColor` is equivalent to the
+/// effective color, which is determined by the [BuildContext] it is last resolved


I think it would be worth pointing out that the "effective" color is this color's [Color.value].

HansMuller · 2019-08-13T00:16:23Z

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

+    return (inheritedTheme?.theme?.data ?? const CupertinoThemeData()).resolveFrom(context, nullOk: true);
+  }
+
+  /// Retrieve the [Brightness] value from an ancestor [CupertinoTheme] widget.


from an => from the closest

HansMuller · 2019-08-13T00:27:08Z

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

+/// when the current [BuildContext] is not available (e.g., in [CupertinoApp]'s
+/// constructor).
+@immutable
+class CupertinoSystemColorsData extends Diagnosticable {


This class should have lerp and debugFillProperties methods, see packages/flutter/lib/src/material/tooltip_theme.dart for examples.

There are some boilerplate-style tests to write for FooData classes like this. See the first 3 tests in packages/flutter/tests/material/tooltip_theme_tests.dart for examples.

What would be the use case of lerping CupertinoSystemColorsData?

Not sure it applies in this case, but in other theme data classes they are usually a field in the overall ThemeData which needs to lerp all of its fields in its own lerp function (which is used for animating theme changes).

I can't really think of a use case where lerping the entire CupertinoSystemColorsData, and on iOS light/dark change doesn't seem to animate

HansMuller · 2019-08-13T00:36:23Z

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

+       assert(data != null),
+       super(key: key, child: child);
+
+  /// Creates a widget that provides a [CupertinoSystemColorsData] extracted from


We haven't provided this sort of thing for other themes (inherited widgets) and it's apt to be confusing as defined because the child will not "see" the CupertinoSystemColors.

Prefer to leave this out now; write a utility in the test file that adds the extra context with a Builder. In the future, perhaps we'll update all themes to include such a factory.

HansMuller · 2019-08-13T00:38:11Z

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

+  /// Falls back to [fromSystem] and then [fallbackValues], if such information
+  /// is not available in the given [BuildContext].
+  static CupertinoSystemColorsData of(BuildContext context) {
+    final CupertinoSystemColors widget = context.inheritFromWidgetOfExactType(CupertinoSystemColors);


The optional parameter is usually called nullOk.

HansMuller · 2019-08-13T00:38:36Z

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

+  ///
+  /// On iOS 13 and above all the colors will be fetched from the operating system.
+  /// On Android and lower versions of iOS this will return null.
+  static CupertinoSystemColorsData get fromSystem => null;


Lets leave it out until it's needed.

HansMuller

LGTM

HansMuller · 2019-08-14T19:13:39Z

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

+  /// Falls back to [fromSystem] and then [fallbackValues], if such information
+  /// is not available in the given [BuildContext].
+  static CupertinoSystemColorsData of(BuildContext context) {
+    final CupertinoSystemColors widget = context.inheritFromWidgetOfExactType(CupertinoSystemColors);


That sounds OK to me.

HansMuller · 2019-08-14T19:42:51Z

packages/flutter/lib/src/widgets/media_query.dart

+  /// * Settings -> Accessibility -> High contrast text (Android)
+  ///
+  /// This flag is currently only updated on iOS devices that are running iOS 13
+  /// or above, and Android devices that are running Android Q and above.


I think we decided that, for now, we're not reporting it for Android Q because it's still an experimental Android feature.

HansMuller · 2019-08-14T19:47:31Z

packages/flutter/test/cupertino/colors_test.dart

+      expect(system0, isNot(withDifferentLink));
+    });
+
+    test('CupertinoSystemColorsData.hasCode', () {


hasCode => hashCode

justinmc

LGTM, just a question for my own sake.

justinmc · 2019-08-15T22:44:24Z

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

+             <Color>[highContrastColor, highContrastElevatedColor],
+           ],
+         ],
+       );


Can you explain this choice of data structure for _colorMap? Is it just that you need to be able to group them and iterate them?

It does seem that this structure adds a complication without much benefit that I can see (but there is a good chance I am missing something here).

Having a 3-dimensional list doesn't document itself very well. For example is the first index based on brightness, elevation or contrast? Does a dark brightness map to a 0 or a 1 index?

Perhaps a simpler approach would be to just treat the CupertinoDynamicColor as a data structure class with fields for each of the colors. The resolve methods would just choose which of the colors are needed based on the context properties. It would lead to a large if-then-else statement to pick which of the 8 colors it resolves to, but it would be explicit. I think it would also mean you wouldn't need the _isPlatformBrightnessDependent type functions as well.

Yes, also in case in the future more traits are needed to resolve a dynamic color, or some traits become non-binary, it's much easier to maintain the resolve method this way, than having a bunch of if statements, or with bit acrobatics.

updated the implementation of the getters, it should be more readable for the getters now.

I think using a 3d list to represent the traits kinda mathematically makes sense, the 3 traits can be thought of as 3 orthogonal bases to form the whole trait space.

Some of the values are enums so it's worse than a few if else's, it's going to be nested switches, because the style guide discourages using == for enum matching.

There're also some negligible optimizations in it:

the CupertinoDynamicColor.resolve method is almost 0 copy because it just passes the reference of the array around.

CupertinoDynamicColor.==is also faster (just realized this and made the change).

justinmc · 2019-08-15T22:55:33Z

packages/flutter/test/cupertino/colors_test.dart

+      highContrastColor: color0,
+      darkHighContrastColor: color0,
+    ));
+  });


Thanks for updating, it's a lot easier to understand now.

darrenaustin

Looks good. Nice work. My main concern is over the data structure used to store the colors in CupertinoDynamicColor. The nested list representation is hard to follow and document.

darrenaustin · 2019-08-14T19:54:05Z

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

+/// When used as a regular color, `CupertinoDynamicColor` is equivalent to the
+/// effective color (i.e. [CupertinoDynamicColor.value] will come from the effective
+/// color), which is determined by the [BuildContext] it is last resolved against. If it has never
+/// been resolved, typically the light, normal contrast, base elevation variant


"typically"? what causes it to not be the light, normal contrast, base elevation variant? Or is this documented in the individual colors?

Right now a CupertinoDynamicColor coming fresh out of a constructor always uses color as its effective color. Removing that "typically".

darrenaustin · 2019-08-14T19:56:09Z

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

+// Color darkModeColor;

 /// A palette of [Color] constants that describe colors commonly used when
 /// matching the iOS platform aesthetics.


Do we want to add a note here that they should probably switch over to using the dynamic and system colors?

Now is probably not the best time, since we're still using the fallback values in CupertinoSystemColors, and we're not even using those new colors ourselves? Maybe as the last step of dark mode?

darrenaustin · 2019-08-16T01:02:15Z

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

+             <Color>[highContrastColor, highContrastElevatedColor],
+           ],
+         ],
+       );


It does seem that this structure adds a complication without much benefit that I can see (but there is a good chance I am missing something here).

Having a 3-dimensional list doesn't document itself very well. For example is the first index based on brightness, elevation or contrast? Does a dark brightness map to a 0 or a 1 index?

Perhaps a simpler approach would be to just treat the CupertinoDynamicColor as a data structure class with fields for each of the colors. The resolve methods would just choose which of the colors are needed based on the context properties. It would lead to a large if-then-else statement to pick which of the 8 colors it resolves to, but it would be explicit. I think it would also mean you wouldn't need the _isPlatformBrightnessDependent type functions as well.

darrenaustin · 2019-08-16T01:04:58Z

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

+    int interfaceElevationNumber = 0;
+
+    // If this CupertinoDynamicColor cares about brightness.
+    if (_isPlatformBrightnessDependent) {


Why do we need the _isPlatformBrightnessDependent needed here? (and similar for the other dependent calls below)

It's to avoid registering unnecessary dependencies. A widget that only uses a brightness independent CupertinoDynamicColor should not be rebuilt when platform brightness changes.

darrenaustin · 2019-08-16T01:08:49Z

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

+/// when the current [BuildContext] is not available (e.g., in [CupertinoApp]'s
+/// constructor).
+@immutable
+class CupertinoSystemColorsData extends Diagnosticable {


Not sure it applies in this case, but in other theme data classes they are usually a field in the overall ThemeData which needs to lerp all of its fields in its own lerp function (which is used for animating theme changes).

…flutter into cupertino-dynamic-color

HansMuller

LGTM

HansMuller · 2019-08-21T22:25:53Z

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


 const double _kPadding = 8.0;
-const Color _kTrackColor = Color(0xFFB5B5B5);
+//const Color _kTrackColor = Color(0xFFB5B5B5);


Looks like you intended to delete this

HansMuller · 2019-08-21T22:41:34Z

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

+  Color _trackColor;
+  set trackColor(Color value) {
+    if (value == _trackColor)
+    return;


missing indent

…flutter into cupertino-dynamic-color

LongCatIsLooong added 26 commits July 30, 2019 13:51

CupertinoDynamicColor

b032d81

add nullOk

e8612db

elevation

d4864d6

indentation

224add7

documentation

05bd0ad

update media query

0bbbfaf

documentation

6eb4470

documentation

a12c4f0

add tests

71057de

update comment

6a42ad9

update comments

b8b3df8

add SystemColorData

5cafcaa

add ==

292f037

update

72b701c

colors

c26602d

color update

338172f

update

4177633

update

803d0f5

add fromCurrentContext

9635074

Exposing the widget

e728079

resolve CupertinoThemeData in of

7e45c5a

update

415c849

docs

c103a44

fix test

7824172

fix tests

17f297a

add tests

4541a67

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 6, 2019

LongCatIsLooong requested a review from HansMuller August 6, 2019 20:53

LongCatIsLooong force-pushed the cupertino-dynamic-color branch 3 times, most recently from 69dc0ea to c702009 Compare August 12, 2019 18:35

make CupertinoSystemColors' default constructor public

f408d63

LongCatIsLooong force-pushed the cupertino-dynamic-color branch from c702009 to f408d63 Compare August 12, 2019 19:36

fix import

ae37dc9

HansMuller reviewed Aug 13, 2019

View reviewed changes

LongCatIsLooong force-pushed the cupertino-dynamic-color branch 3 times, most recently from 8a03a53 to 415c8a3 Compare August 14, 2019 18:24

review

039edf7

LongCatIsLooong force-pushed the cupertino-dynamic-color branch from 415c8a3 to 039edf7 Compare August 14, 2019 19:25

HansMuller approved these changes Aug 14, 2019

View reviewed changes

review

f3db6b8

justinmc approved these changes Aug 15, 2019

View reviewed changes

darrenaustin reviewed Aug 16, 2019

View reviewed changes

readability

d45350e

LongCatIsLooong force-pushed the cupertino-dynamic-color branch from d1117f8 to d45350e Compare August 16, 2019 03:29

LongCatIsLooong added 2 commits August 20, 2019 16:55

Apply new colors to CupertinoSlider

e6d2dc9

Merge branch 'cupertino-dynamic-color' of github.com:LongCatIsLooong/…

4f4a2aa

…flutter into cupertino-dynamic-color

HansMuller approved these changes Aug 21, 2019

View reviewed changes

LongCatIsLooong added 4 commits August 21, 2019 15:49

Update

f1ce85f

Update slider.dart

fee5ab2

Merge branch 'cupertino-dynamic-color' of github.com:LongCatIsLooong/…

db10616

…flutter into cupertino-dynamic-color

Merge branch 'cupertino-dynamic-color' of github.com:LongCatIsLooong/…

5202fc6

…flutter into cupertino-dynamic-color

LongCatIsLooong merged commit a671b28 into flutter:master Aug 22, 2019

LongCatIsLooong deleted the cupertino-dynamic-color branch August 22, 2019 03:07

justinmc mentioned this pull request Sep 6, 2019

Planned Cupertino Library Features Jul-Sept 2019 #35026

Closed

6 tasks

HansMuller mentioned this pull request Oct 18, 2019

Upcoming Cupertino Library Features #43041

Closed

18 tasks

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

		@@ -1,8 +1,21 @@
		// Copyright 2017 The Chromium Authors. All rights reserved.
		// Copyright 2019 The Chromium Authors. All rights reserved.

Conversation

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

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

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

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

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!

darrenaustin 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

LongCatIsLooong commented Aug 6, 2019 •

edited

Loading