make CupertinoDynamicColor const constructible by LongCatIsLooong · Pull Request #39430 · flutter/flutter

LongCatIsLooong · 2019-08-28T20:03:50Z

Description

make CupertinoDynamicColor's constructors const, in order to use a CupertinoDynamicColor as the default value of a function argument. This change would allow us to provide a default CupertinoDynamicColor in the constructor of the following widget, without breaking its current API:

class SomeWidget extends StatelessWidget {
  const SomeWidget({ Key key, this.color = defaultColor }) : super(key: key);
}

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.

Tests

No new tests were added: existing tests should be able to cover this.

Breaking Change

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

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

LongCatIsLooong · 2019-08-28T20:15:12Z

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

+      : CupertinoUserInterfaceLevelData.base;
+
+    Color resolved;
+    switch (brightness) {


I was expecting a much larger switch statement

Yeah with only eight possibilities (at least for now) it isn't too bad. It is a bit verbose, but I think it gains a lot in clarity.

I am scared of enums and switches, especially when the enum has a name like CupertinoUserInterfaceLevelData

justinmc

LGTM

It's great that you were able to make this const. Did you find that it needed to be const for some reason or are you just doing it because it's possible?

justinmc · 2019-08-28T21:27:14Z

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

+         elevatedColor,
+         darkElevatedColor,
+         highContrastElevatedColor,
+         darkHighContrastElevatedColor,


I think this is a lot more clear 👍

Indeed. Nice!

justinmc · 2019-08-28T21:30:14Z

packages/flutter/test/cupertino/colors_test.dart

        highContrastColor: color2,
        darkHighContrastColor: color3,
-    ));
+      )


Comma here?

LongCatIsLooong · 2019-08-28T21:45:02Z

@justinmc default values have to be const values, if a widget's constructor has a color parameter with a default color, it can't take a CupertinoDynamicColor as the default value if CupertinoDynamicColor doesn't have a const constructor. I was converting cupertino widgets and realized that CupertinoDynamicColor being not const constructible is going to break quite a few widgets' current API so I came back and changed it. I had to call the super constructor with a dummy value though:

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

Line 227 in 20009df

super(0);

HansMuller · 2019-08-28T21:44:19Z

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

+      assert(highContrastElevatedColor != null),
+      assert(darkHighContrastElevatedColor != null),
+      assert(_effectiveColor != null),
+      super(0);


A // comment that refers to the value getter would be useful here.

HansMuller · 2019-08-28T21:44:30Z

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

-      }(), 'The colorMap provided is invalid: $_colorMap'),
-      super(value.value);
+  @override
+  int get value => _effectiveColor.value;


HansMuller · 2019-08-28T21:46:33Z

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

+            resolved = isHighContrastEnabled ? highContrastColor : color;
+            break;
+          case CupertinoUserInterfaceLevelData.elevated:
+            resolved = isHighContrastEnabled ? highContrastElevatedColor : elevatedColor;


add a perfunctory break here

HansMuller · 2019-08-28T21:48:40Z

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

@@ -425,7 +437,14 @@ class CupertinoDynamicColor extends Color {

    return other.runtimeType == runtimeType


Maybe check for identical(this, other) first?

It already does.

Sorry, yes it does! Hazards of looking through the github review microscope.

HansMuller · 2019-08-28T21:49:44Z

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

+// Fallback System Colors, extracted from:
+// https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors
+// and iOS 13 beta.
+const CupertinoSystemColorsData _kSystemColorsFallback = CupertinoSystemColorsData(


HansMuller · 2019-08-28T21:56:01Z

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

I'm not sure this is entirely safe. Not sure if developers should expect it either (we're not documenting this case). If resolved.runtimeType != value.runtimeType do we still want to do this? Seems like it would be easier to understand the semantics of thie method if we unconditionally returned a CupertinoDynamicColor (not a subclass).

darrenaustin

LGTM. Nice update. Having a const constructor for a class like this is helpful and the changes to the color storage are a lot easier to follow.

darrenaustin · 2019-08-28T22:07:20Z

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

+         elevatedColor,
+         darkElevatedColor,
+         highContrastElevatedColor,
+         darkHighContrastElevatedColor,


Indeed. Nice!

darrenaustin · 2019-08-28T22:09:38Z

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

+      : CupertinoUserInterfaceLevelData.base;
+
+    Color resolved;
+    switch (brightness) {


Yeah with only eight possibilities (at least for now) it isn't too bad. It is a bit verbose, but I think it gains a lot in clarity.

HansMuller

LGTM

make CupertinoDynamicColor const constructible

20009df

LongCatIsLooong requested review from HansMuller, darrenaustin and justinmc August 28, 2019 20:03

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

googlebot added the cla: yes label Aug 28, 2019

LongCatIsLooong commented Aug 28, 2019

View reviewed changes

justinmc approved these changes Aug 28, 2019

View reviewed changes

justinmc reviewed Aug 28, 2019

View reviewed changes

HansMuller reviewed Aug 28, 2019

View reviewed changes

darrenaustin approved these changes Aug 28, 2019

View reviewed changes

review

2ab56b8

LongCatIsLooong force-pushed the const-constructible-dynamic-color branch from 823fa8c to 2ab56b8 Compare August 28, 2019 22:17

HansMuller approved these changes Aug 28, 2019

View reviewed changes

LongCatIsLooong merged commit 98b9f31 into flutter:master Aug 29, 2019

LongCatIsLooong deleted the const-constructible-dynamic-color branch August 29, 2019 23:40

justinmc mentioned this pull request Sep 6, 2019

Planned Cupertino Library Features Jul-Sept 2019 #35026

Closed

6 tasks

LongCatIsLooong mentioned this pull request Sep 20, 2019

Remove CupertinoSystemColors in favor of CupertinoColors #40566

Merged

8 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

		@@ -425,7 +437,14 @@ class CupertinoDynamicColor extends Color {

		return other.runtimeType == runtimeType

Conversation

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

Uh oh!

Description

Checklist

Tests

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

LongCatIsLooong commented Aug 28, 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!

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!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LongCatIsLooong commented Aug 28, 2019 •

edited

Loading

LongCatIsLooong Aug 28, 2019 •

edited

Loading