feat: implement unified IFormattable on Color, HslColor, and HsvColor#20919
feat: implement unified IFormattable on Color, HslColor, and HsvColor#20919NathanDrake2406 wants to merge 3 commits intoAvaloniaUI:masterfrom
Conversation
|
You can test this PR using the following package version. |
|
Nice! Couple questions:
|
bfef8c5 to
7adf64c
Compare
|
2d694b5 to
f2855be
Compare
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
This is a small PR in code and it's all a related discussion. Sometimes its useful to keep all discussions in one place ;)
Awesome! This should help developers and maintain consistency. More below about specifier codes. There is another deeper conversation here. In .NET itself For colors we have So if you want to output an This relates to #20928 (comment) (again, it's all the same discussion so I'm keeping it here in one spot). What do you think about this line of thought? As a spec this means ALL color types would have the exact same formatting codes and be treated exactly the same.
Things now expand pretty quickly when you are trying to support formatting of 1) all color models, 2) absolute/percent and 3) with/without alpha. The number of permutations expand quite a bit and for this reason I think we should go to two-letter format codes (which differs from your latest changes in code). Going to multiple letters keeps things readable and avoids having to come up with new letters like "P/p" that we might need in the future. The format specifier is as follows:
This can be expanded the same for
Now I need to talk about the complex formatting codes. While at first it doesn't seem like it should be done now it's relevant for two reasons:
A full complex format code would be something like
The obvious downside here is the complexity of parsing this grammar. It does support all cases though. However, if we keep this in mind and that we should reserve "C" and "A" for future use like this I think we are OK. The current design is future-proof. Finally we have a few new issues:
This is blazing a brand-new trail so is getting a lot of thought typical of what you find in the .NET core lib but not necessarily Avalonia itself for this type of thing. Hopefully I'm not scaring you with the depth here. |
Pre-existing copy-paste bug: HslColor.ToString() was outputting
"hsva(" instead of "hsla(" for its default format.
All three color types now support all format specifiers via auto-conversion, following the DateTime analogy where the same format codes work regardless of source type. Format specifiers: Hex: X (#AARRGGBB), x (#RRGGBB), H (#RRGGBBAA) RGB: R/r (absolute), R%/r% (percent) HSL: L/l (CSS standard), L%/l% (all percent) HSV: V/v (CSS standard), V%/v% (all percent) Convention: uppercase = include alpha (rgba/hsla/hsva prefix), lowercase = exclude alpha (rgb/hsl/hsv prefix). Each type handles its native model natively and delegates cross-model formats via ToRgb()/ToHsl()/ToHsv(). Breaking changes from prior PR iteration: - "h" dropped (identical to "x" without alpha) - "C"/"c" removed, reserved for future complex format strings - "P"/"p" replaced by "R%"/"r%" - "R" now outputs rgba() not rgb() (CSS convention) - "L" now outputs hsla() not hsl() (CSS convention) - "V" now outputs hsva() not hsv() (CSS convention)
744d0d0 to
0b0dd03
Compare
|
You can test this PR using the following package version. |
|
@NathanDrake2406 I don't think this is doing to die at all:
Separately, I have to go back and see what changes you made. Note that just saying "updated" or similar slows down the review. I spent a lot of time on my feedback and having to go back through to figure out what exactly you changed takes additional time I don't always have. I do want to understand the decisions you made around CSS formatting. It's probably the right decision to always default to CSS-compliant formats; but that does deviate from pure absolute/percent formats we originally discussed above.
I'm glad you kept this architecture. It aligns with the original design and simplifies the code quite a bit. |
Summary
Implements
IFormattableon all three color types (Color,HslColor,HsvColor) with a unified set of format specifiers — any type can output any format via auto-conversion, following theDateTimeanalogy discussed in review.Also fixes a pre-existing bug:
HslColor.ToString()was outputtinghsva(instead ofhsla(.Closes #18725
Design
Convention: uppercase = include alpha (
rgba/hsla/hsvaprefix), lowercase = exclude alpha (rgb/hsl/hsvprefix).%suffix = percent mode.null/""Red,hsla(230, 1, 0.5, 1)"X"#FFFF0000"x"#FF0000"H"#FF0000FF"R"rgba(255, 0, 0, 1.00)"r"rgb(255, 0, 0)"R%"rgba(100%, 0%, 0%, 100%)"r%"rgb(100%, 0%, 0%)"L"hsla(0, 100%, 50%, 1.00)"l"hsl(0, 100%, 50%)"L%"hsla(0%, 100%, 50%, 100%)"l%"hsl(0%, 100%, 50%)"V"hsva(0, 100%, 100%, 1.00)"v"hsv(0, 100%, 100%)"V%"hsva(0%, 100%, 100%, 100%)"v%"hsv(0%, 100%, 100%)"C"and"A"are reserved for future complex format strings (hsv:C1,C2,C3,A).Architecture
Each type only implements its native format natively and delegates cross-model formats via conversion:
Colorhandles hex + RGB natively; delegatesL/V→ToHsl()/ToHsv()HslColorhandles HSL natively; delegates hex/RGB →ToRgb(), HSV →ToHsv()HsvColorhandles HSV natively; delegates hex/RGB →ToRgb(), HSL →ToHsl()Zero duplicated formatting logic. Every delegation terminates in one hop.
Breaking changes from prior iteration
"h"dropped (identical to"x")"C"/"c"removed, reserved for future use"P"/"p"replaced by"R%"/"r%"rgba()/hsla()/hsva()prefix (CSS convention)Test plan
HslColor.ToString("X")→ hex via RGB)C,c,A,a) throwFormatExceptionh,P,p) throwFormatExceptionIFormatProviderignored (culture-invariant verified with fr-FR)ToString()unchanged for backwards compatibilityhsla()bug fix verified (washsva())