Skip to content

Commit 0bdad6f

Browse files
benthillerkusbenthillerkus
authored
Change saturation calculation for HSLColor.fromColor (flutter#166639)
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> This pull request aligns the calculation of saturation for RGB to HSV conversion with the common formulas you find on the [internet](https://en.wikipedia.org/wiki/HSL_and_HSV#Saturation) (concrete: if `r`, `g` and `b` are the same (-> gray), the saturation should be 0). This fixes black currently having a saturation of `1` instead of `0`. fixes flutter#166460 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent 29810cd commit 0bdad6f

File tree

2 files changed

+121
-3
lines changed

2 files changed

+121
-3
lines changed

‎packages/flutter/lib/src/painting/colors.dart

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,7 @@ class HSLColor {
266266
final double lightness = (max + min) / 2.0;
267267
// Saturation can exceed 1.0 with rounding errors, so clamp it.
268268
final double saturation =
269-
lightness == 1.0
270-
? 0.0
271-
: clampDouble(delta / (1.0 - (2.0 * lightness - 1.0).abs()), 0.0, 1.0);
269+
min == max ? 0.0 : clampDouble(delta / (1.0 - (2.0 * lightness - 1.0).abs()), 0.0, 1.0);
272270
return HSLColor.fromAHSL(alpha, hue, saturation, lightness);
273271
}
274272

‎packages/flutter/test/painting/colors_test.dart

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,44 @@ void main() {
323323
expect(output, equals(expectedColors));
324324
});
325325

326+
group('HSLColor.fromColor tests', () {
327+
test('Pink', () {
328+
const Color color = Color.fromARGB(255, 255, 51, 152);
329+
final HSLColor hslColor = HSLColor.fromColor(color);
330+
expect(hslColor.alpha, 1.0);
331+
expect(hslColor.hue, within<double>(distance: .3, from: 330));
332+
expect(hslColor.saturation, 1.0);
333+
expect(hslColor.lightness, within<double>(distance: _doubleColorPrecision, from: 0.6));
334+
});
335+
336+
test('White', () {
337+
const Color color = Color(0xffffffff);
338+
final HSLColor hslColor = HSLColor.fromColor(color);
339+
expect(hslColor.alpha, 1.0);
340+
expect(hslColor.hue, 0.0);
341+
expect(hslColor.saturation, 0.0);
342+
expect(hslColor.lightness, 1.0);
343+
});
344+
345+
test('Black', () {
346+
const Color color = Color(0xff000000);
347+
final HSLColor hslColor = HSLColor.fromColor(color);
348+
expect(hslColor.alpha, 1.0);
349+
expect(hslColor.hue, 0.0);
350+
expect(hslColor.saturation, 0.0);
351+
expect(hslColor.lightness, 0.0);
352+
});
353+
354+
test('Gray', () {
355+
const Color color = Color(0xff808080);
356+
final HSLColor hslColor = HSLColor.fromColor(color);
357+
expect(hslColor.alpha, 1.0);
358+
expect(hslColor.hue, 0.0);
359+
expect(hslColor.saturation, 0.0);
360+
expect(hslColor.lightness, within<double>(distance: _doubleColorPrecision, from: 0.5));
361+
});
362+
});
363+
326364
test('HSLColor.lerp identical a,b', () {
327365
expect(HSLColor.lerp(null, null, 0), null);
328366
const HSLColor color = HSLColor.fromAHSL(1.0, 0.0, 0.5, 0.5);
@@ -414,6 +452,88 @@ void main() {
414452
expect(output, equals(expectedColors));
415453
});
416454

455+
// Tests the implementation against these colors from Wikipedia
456+
// https://en.wikipedia.org/wiki/HSL_and_HSV#Examples
457+
test('Wikipedia Examples Table test', () {
458+
// ignore: always_specify_types
459+
final colors = <(int, double, double, double, double, double, double, double, double, String)>[
460+
// RGB, r, g, b, hue, v, l,s(hsv),s(hsl)
461+
(0xFFFFFFFF, 1.0, 1.0, 1.0, 0.0, 1.0, 1.0, 0.0, 0.0, 'white'),
462+
(0xFF808080, 0.5, 0.5, 0.5, 0.0, 0.5, 0.5, 0.0, 0.0, 'gray'),
463+
(0xFF000000, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 'black'),
464+
(0xFFFF0000, 1.0, 0.0, 0.0, 0.0, 1.0, 0.5, 1.0, 1.0, 'red'),
465+
(0xFFBFBF00, .75, .75, 0.0, 60, .75, .375, 1.0, 1.0, 'lime'),
466+
(0xFF008000, 0.0, 0.5, 0.0, 120, 0.5, .25, 1.0, 1.0, 'green'),
467+
(0xFF80FFFF, 0.5, 1.0, 1.0, 180, 1.0, 0.75, 0.5, 1, 'cyan'),
468+
(0xFF8080FF, 0.5, 0.5, 1.0, 240, 1.0, 0.75, 0.5, 1, 'light purple'),
469+
(0xFFBF40BF, 0.75, 0.25, 0.75, 300, .75, .5, 2.0 / 3, .5, 'mute magenta'),
470+
];
471+
472+
for (final (
473+
int rgb,
474+
double r,
475+
double g,
476+
double b,
477+
double hue,
478+
double v,
479+
double l,
480+
double sHSV,
481+
double sHSL,
482+
String name,
483+
)
484+
in colors) {
485+
final Color color = Color.from(alpha: 1.0, red: r, green: g, blue: b);
486+
final String debugColorConstructor = 'Color.from(alpha: 1.0, red: $r, green: $g, blue: $b)';
487+
final Color intColor = Color(rgb);
488+
expect(
489+
intColor.r,
490+
within<double>(distance: _doubleColorPrecision, from: r),
491+
reason: '$name: Color($rgb).r should be $r',
492+
);
493+
expect(
494+
intColor.g,
495+
within<double>(distance: _doubleColorPrecision, from: g),
496+
reason: '$name: Color($rgb).g should be $g',
497+
);
498+
expect(
499+
intColor.b,
500+
within<double>(distance: _doubleColorPrecision, from: b),
501+
reason: '$name: Color($rgb).b should be $b',
502+
);
503+
final HSVColor hsv = HSVColor.fromAHSV(1.0, hue, sHSV, v);
504+
final HSLColor hsl = HSLColor.fromAHSL(1.0, hue, sHSL, l);
505+
expect(
506+
color,
507+
within<Color>(distance: _doubleColorPrecision, from: intColor),
508+
reason: '$name: $debugColorConstructor should be close to Color($rgb)',
509+
);
510+
expect(
511+
hsv.toColor(),
512+
within<Color>(distance: _doubleColorPrecision, from: color),
513+
reason:
514+
'$name: HSVColor.fromAHSV(1.0, $hue, $sHSV, $v).hsv should be close to $debugColorConstructor',
515+
);
516+
expect(
517+
hsl.toColor(),
518+
within<Color>(distance: _doubleColorPrecision, from: color),
519+
reason:
520+
'$name: HSLColor.fromAHSL(1.0, $hue, $sHSL, $l).hsl should be close to $debugColorConstructor',
521+
);
522+
expect(
523+
HSVColor.fromColor(color),
524+
within<HSVColor>(distance: _doubleColorPrecision, from: hsv),
525+
reason:
526+
'$name: HSVColor.fromColor($debugColorConstructor) should be close to HSVColor.fromAHSV(1.0, $hue, $sHSV, $v)',
527+
);
528+
expect(
529+
HSLColor.fromColor(color),
530+
within<HSLColor>(distance: _doubleColorPrecision, from: hsl),
531+
reason:
532+
'$name: HSLColor.fromColor($debugColorConstructor) should be close to HSLColor.fromAHSL(1.0, $hue, $sHSL, $l)',
533+
);
534+
}
535+
});
536+
417537
test('ColorSwatch test', () {
418538
final int color = nonconst(0xFF027223);
419539
final ColorSwatch<String> greens1 = ColorSwatch<String>(color, const <String, Color>{

0 commit comments

Comments
 (0)