-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
add ColorFilter.toString to web_ui #43874
Conversation
|
||
@override | ||
String toString() { | ||
switch (type) { |
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.
could be switch expression!
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 could be an if chain too :-P
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 a switch expression is better.
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.
why?
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's tighter. It makes it trivial to parse the intent. You KNOW every branch yields a value. Greatly increases scannability of the code IMHO
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 don't think there's a meaningful difference in readability between these two:
String toString() {
switch (type) {
case ColorFilterType.mode:
return 'ColorFilter.mode($color, $blendMode)';
case ColorFilterType.matrix:
return 'ColorFilter.matrix($matrix)';
case ColorFilterType.linearToSrgbGamma:
return 'ColorFilter.linearToSrgbGamma()';
case ColorFilterType.srgbToLinearGamma:
return 'ColorFilter.srgbToLinearGamma()';
}
}
String toString() {
return switch (type) {
ColorFilterType.mode => 'ColorFilter.mode($color, $blendMode)',
ColorFilterType.matrix => 'ColorFilter.matrix($matrix)',
ColorFilterType.linearToSrgbGamma => 'ColorFilter.linearToSrgbGamma()',
ColorFilterType.srgbToLinearGamma => 'ColorFilter.srgbToLinearGamma()',
};
}
In both cases the language enforces that there must be a return. In both cases it scans relatively easily (maybe the first is a little easier, in the second the return
is not as obvious because of being on the same line as the switch
, but maybe the second is easier because of the avoidance of line wrapping).
The former in general is better IMHO in this case just because it's more consistent with other code in the same package (it's literally just copied and pasted from the existing code elsewhere), and because it's easier to maintain if we ever have to add a branch that's more complicated than just a single string return.
…131218) flutter/engine@815b971...a489c74 2023-07-24 skia-flutter-autoroll@skia.org Roll Skia from 99e8dc51ba53 to 6c219acc30a5 (4 revisions) (flutter/engine#43970) 2023-07-24 ian@hixie.ch add ColorFilter.toString to web_ui (flutter/engine#43874) 2023-07-24 skia-flutter-autoroll@skia.org Roll ANGLE from 5e21d7f02425 to 2d999f744809 (1 revision) (flutter/engine#43968) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#131218) flutter/engine@815b971...a489c74 2023-07-24 skia-flutter-autoroll@skia.org Roll Skia from 99e8dc51ba53 to 6c219acc30a5 (4 revisions) (flutter/engine#43970) 2023-07-24 ian@hixie.ch add ColorFilter.toString to web_ui (flutter/engine#43874) 2023-07-24 skia-flutter-autoroll@skia.org Roll ANGLE from 5e21d7f02425 to 2d999f744809 (1 revision) (flutter/engine#43968) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#131218) flutter/engine@815b971...a489c74 2023-07-24 skia-flutter-autoroll@skia.org Roll Skia from 99e8dc51ba53 to 6c219acc30a5 (4 revisions) (flutter/engine#43970) 2023-07-24 ian@hixie.ch add ColorFilter.toString to web_ui (flutter/engine#43874) 2023-07-24 skia-flutter-autoroll@skia.org Roll ANGLE from 5e21d7f02425 to 2d999f744809 (1 revision) (flutter/engine#43968) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
No description provided.