feat: implement unified IFormattable on Color, HslColor, and HsvColor - #20919
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
NathanDrake2406
changed the title
|
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. |
I’ll summarise changes properly next time. I deviated there because I wanted the common case to be immediately recognisable in the format most Avalonia devs would expect. Since this is a UI framework, CSS style output felt like the more natural default. The pure absolute/percent model was cleaner as a spec, but % still keeps the all-percent form available when anyone wants to use it |
|
You can test this PR using the following package version. |
MrJul
added
the
api-needs-review
robloo
left a comment
robloo
left a comment
There was a problem hiding this comment.
I've gone over all this again and looked at the code.
- I agree with your decision to make the default formats CSS compliant. That is most universal and will make the most sense rather than making up our own "Absolute" variants.
- Aside from the
#if !BUILDTASKthe code looks fully functional and to spec -- can be merged as-is. - I did add some ideas for simplification and to make code more explicit for future developers. However, that can be done in a later PR.
I don't see any issues with the ideas and implementation here. What is needed to get this through the API review? On my end (as the author of the HSL/HSV color structs, color picker) I sign off on this PR.
| private string FormatRgbaPercent() | ||
| { | ||
| int rPct = (int)Math.Round(R * byteToDouble * 100.0); | ||
| int gPct = (int)Math.Round(G * byteToDouble * 100.0); | ||
| int bPct = (int)Math.Round(B * byteToDouble * 100.0); | ||
| int aPct = (int)Math.Round(A * byteToDouble * 100.0); | ||
| return string.Format(CultureInfo.InvariantCulture, "rgba({0}%, {1}%, {2}%, {3}%)", rPct, gPct, bPct, aPct); | ||
| } | ||
|
|
||
| private string FormatRgbPercent() | ||
| { | ||
| int rPct = (int)Math.Round(R * byteToDouble * 100.0); | ||
| int gPct = (int)Math.Round(G * byteToDouble * 100.0); | ||
| int bPct = (int)Math.Round(B * byteToDouble * 100.0); | ||
| return string.Format(CultureInfo.InvariantCulture, "rgb({0}%, {1}%, {2}%)", rPct, gPct, bPct); | ||
| } |
There was a problem hiding this comment.
These and similar methods for HsvColor/HslColor can be simplified. As in other code just add an includeAlpha parameter and the internal method can switch between modes. IMO less methods is better.
/// <summary>
/// Formats the color as a string with all components a percentage.
/// </summary>
/// <param name="includeAlpha">Whether the alpha component will be included in the string.</param>
private string FormatRgbPercent(bool includeAlpha = true)
{
int rPct = (int)Math.Round(R * byteToDouble * 100.0);
int gPct = (int)Math.Round(G * byteToDouble * 100.0);
int bPct = (int)Math.Round(B * byteToDouble * 100.0);
if (includeAlpha)
{
int aPct = (int)Math.Round(A * byteToDouble * 100.0);
return string.Format(CultureInfo.InvariantCulture, "rgba({0}%, {1}%, {2}%, {3}%)", rPct, gPct, bPct, aPct);
}
else
{
return string.Format(CultureInfo.InvariantCulture, "rgb({0}%, {1}%, {2}%)", rPct, gPct, bPct);
}
}| "R" => string.Format(CultureInfo.InvariantCulture, "rgba({0}, {1}, {2}, {3:F2})", R, G, B, A * byteToDouble), | ||
| "r" => string.Format(CultureInfo.InvariantCulture, "rgb({0}, {1}, {2})", R, G, B), |
There was a problem hiding this comment.
We should probably add comments making it VERY clear these are CSS compliant formats.
| private string FormatHsla() | ||
| { | ||
| int hDeg = (int)Math.Round(H); | ||
| int sPct = (int)Math.Round(S * 100.0); | ||
| int lPct = (int)Math.Round(L * 100.0); | ||
| return string.Format(CultureInfo.InvariantCulture, "hsla({0}, {1}%, {2}%, {3:F2})", hDeg, sPct, lPct, A); | ||
| } | ||
|
|
||
| private string FormatHsl() | ||
| { | ||
| int hDeg = (int)Math.Round(H); | ||
| int sPct = (int)Math.Round(S * 100.0); | ||
| int lPct = (int)Math.Round(L * 100.0); | ||
| return string.Format(CultureInfo.InvariantCulture, "hsl({0}, {1}%, {2}%)", hDeg, sPct, lPct); | ||
| } |
There was a problem hiding this comment.
Again these methods can be combined. However, I think we also need to rename them to make is COMPLETELY CLEAR that the resulting format is CSS compliant. This will prevent other developers from accidentally breaking this concept in the future.
As an example TryParseCssFormat already exists in Color.
string FormatHslCss(bool includeAlpha) makes sense to me.
| #if !BUILDTASK | ||
| , IFormattable | ||
| #endif |
There was a problem hiding this comment.
I don't think it's really necessary to do this. IFormattable is part of System so is fully supported by the BuildTask. Even so it will just be ignored as the original .ToString() methods will be called instead.
Color formatting now exposes the same IFormattable implementation in the BuildTask compile path as in normal builds. Keeping the interface conditional made the public contract harder to reason about even though the formatting members are System-only APIs. Remove the BuildTask guard from the formatting members, combine alpha and no-alpha CSS helpers behind includeAlpha parameters, and rename the helpers so future changes preserve the CSS-compliant output contract.
|
You can test this PR using the following package version. |
I've incorporated your ideas. Just wanna say that I really appreciate your thoughtfulness and extremely strong feedback. It's so rare in OSS. |
|
We've reviewed this, and the feature itself is perfectly fine. However, there were slight concerns about the various format strings not being very readable. We're aware that they're always a bit arbitrary, even in .NET itself, especially when they're not composable (like here or in A proposal was to simply use the name directly as a format specifier, as they're quite short. Nothing was really decided, so I'm posting this for feedback. |
Well, yes, they are somewhat arbitrary. That's why I wondered at first if there was prior art we could fall back to. There wasn't. So, as you alluded to, this was done in the spirit of the And while they do seem arbitrary at first it does start to make sense when you look at it deeper:
I would consider these reserved for future composability (the complex formats). For example it may be needed in the future to support Then there is the generalized version of this idea above:
So the arguments are:
|
|
@robloo I’ve invited you as a collaborator on my fork, this PR is yours if you want to take over. |
MrJul
left a comment
MrJul
left a comment
There was a problem hiding this comment.
Let's go with the proposed formats, but please remove the #region blocks.
MrJul
removed
the
api-needs-review
MrJul
left a comment
MrJul
left a comment
There was a problem hiding this comment.
The regions need to be removed, or the PR allowed to be edited by maintainers.
|
You should be able to edit now. Sorry I’ve been busy, will update the other PRs as well when I have time |
|
@nathan Nguyen, Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following: Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by AvaloniaUI OÜ and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant AvaloniaUI OÜ, and those who receive the Submission directly b. Patent License. You grant AvaloniaUI OÜ, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to AvaloniaUI OÜ. You agree to notify AvaloniaUI OÜ in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the Republic of Estonia, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and AvaloniaUI OÜ dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. 3 out of 4 committers have signed the CLA.
Nathan Nguyen doesn't seem to be a GitHub user. |
Thank you!
And no problem, your contributions are appreciated :) |
|
You can test this PR using the following package version. |
6 participants
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())