Repository navigation
Fix a batch of rough edges around the pickers - #339
Merged
Merged
Conversation
isInteracting was a single Boolean, so with two sliders under two fingers the first to finish answered for the one still moving. It counts now, and SliderInteractionGuard counts each component in once however many times onValueChange fires. fromInt clamped hue, so 370 became 360 and then 0, and -10 became 0. Both answered red, which is a plausible wrong colour rather than a rejected one β the failure a caller stepping a hue past the end would be least likely to notice. Hue is an angle and wraps: 370 is 10, -10 is 350. HslColor, OkhslColor, OkhsvColor and OklchColor all did it. The Independent saturation track started at Compose's Color.Gray, #888888, where saturation zero at mid lightness is #808080 β a step off the colour its own thumb was painted with. Both ends go through the conversion now. The other hardcoded stops were checked and are right: black, white, red, green and blue really are what those positions produce. Origin tracking keeps a hue while the space holding it is the one being written to, but a write from another space through a grey lost it: HSL sliders and an RGB field over one state, drag RGB to grey, and the hue slider jumped to red. A conversion cannot invent what the colour does not carry, so the last hue that was actually chosen is kept and handed back, the way a painting tool does. HSL's hue angle and Oklab's are different quantities and are remembered apart. Saturation is not treated this way: a grey really is unsaturated, where its hue is only unknown. CMYK is the textbook formula with no colour profile, which round-trips on screen and is not what a press will print. Nothing said so. Two LAB tests were loose enough to pass on wrong code. midGrayLabToRgb allowed 0.1 to 0.6 and called 0.184 the sRGB value of L* 50; that is its linear luminance, the encoded value is 0.4663, and a range holding both would pass with the gamma step removed altogether. It pins the encoded value now. rgbRedToLab only asked for 50 < L* < 56 on an input another test already pins to a hundredth, so it is gone rather than kept as a weaker way to fail the same thing. Every picker also takes a colour and a callback now, for an app holding the value in a view model rather than a ColorPickerState. The binding compares in both directions β a value written in is not reported back out β which is what keeps the two ends from updating each other indefinitely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
isInteracting was a single Boolean, so with two sliders under two fingers the first to finish answered for the one still moving. It counts now, and SliderInteractionGuard counts each component in once however many times onValueChange fires.
fromInt clamped hue, so 370 became 360 and then 0, and -10 became 0. Both answered red, which is a plausible wrong colour rather than a rejected one β the failure a caller stepping a hue past the end would be least likely to notice. Hue is an angle and wraps: 370 is 10, -10 is 350. HslColor, OkhslColor, OkhsvColor and OklchColor all did it.
The Independent saturation track started at Compose's Color.Gray, #888888, where saturation zero at mid lightness is #808080 β a step off the colour its own thumb was painted with. Both ends go through the conversion now. The other hardcoded stops were checked and are right: black, white, red, green and blue really are what those positions produce.
Origin tracking keeps a hue while the space holding it is the one being written to, but a write from another space through a grey lost it: HSL sliders and an RGB field over one state, drag RGB to grey, and the hue slider jumped to red. A conversion cannot invent what the colour does not carry, so the last hue that was actually chosen is kept and handed back, the way a painting tool does. HSL's hue angle and Oklab's are different quantities and are remembered apart. Saturation is not treated this way: a grey really is unsaturated, where its hue is only unknown.
CMYK is the textbook formula with no colour profile, which round-trips on screen and is not what a press will print. Nothing said so.
Two LAB tests were loose enough to pass on wrong code. midGrayLabToRgb allowed 0.1 to 0.6 and called 0.184 the sRGB value of L* 50; that is its linear luminance, the encoded value is 0.4663, and a range holding both would pass with the gamma step removed altogether. It pins the encoded value now. rgbRedToLab only asked for 50 < L* < 56 on an input another test already pins to a hundredth, so it is gone rather than kept as a weaker way to fail the same thing.
Every picker also takes a colour and a callback now, for an app holding the value in a view model rather than a ColorPickerState. The binding compares in both directions β a value written in is not reported back out β which is what keeps the two ends from updating each other indefinitely.