gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.54k stars 680 forks source link

Color ToString returns "Red" instead of "# #3603

Open tznind opened 1 month ago

tznind commented 1 month ago

Describe the bug

ToString with a format parameter should result in consistent output format regardless of the constructor values given to it

It seems that the Format process falls back to ToString (parameterless) which will use the name of the color at times for specific hex codes.

Regardless of the behaviour of ToString the Format overload should be returning consistent format i.e. #RRGGBB

I encountered this in writing a Hex textbox for ColorPicker, having it flip to "Red" half way through operation or with certain constructor args is unexpected.

 [Fact]
 public void Color_ToString_g_ShouldBeConsistent_With_G()
 {
     // Arrange
     var color = new Color (255, 0, 0); // Red

     var colorString = color.ToString ("G");
     Assert.Equal ("#FFFF0000", colorString);

     // Works
     colorString = color.ToString ("g");
     Assert.Equal ("#FF0000", colorString);
 }
[Fact]
public void Color_ToString_g_ShouldBeConsistent_With_G_Regardless_Constructor ()
{
    // Arrange
    var color = new Color (197,15,31); // Red

    var colorString = color.ToString ("G");
    Assert.Equal ("#FFC50F1F", colorString);

    // Fails
    colorString = color.ToString ("g");
    Assert.Equal ("#C50F1F", colorString);
}

Test failure is:

Xunit.Sdk.EqualException
Assert.Equal() Failure: Strings differ
           ↓ (pos 0)
Expected: "#C50F1F"
Actual:   "Red"
           ↑ (pos 0)
   at 
dodexahedron commented 1 month ago

Hm. It's partially based on the .net code for Color.

So my question before I do anything with it would be: Is that different behavior from System.Drawing.Color (aside from the custom formatting strings ours supports, of course)?

Thing is that a Color is a Color and that's it. Just a 32-bit little-endian binary integer value. Metadata about the color isn't stored, just like the number of decimal places you want a floating point written out with ToString() isn't stored and is your responsibility to provide at the point of use. Colors aren't strings. That's just a schema for formatting and parsing, which is how .net types work in general.

An idea, if you want a settable custom behavior: I could maybe expose a configurable setting - not in the Color type itself - that Color refers to in the ToString fallback case? You know, something like a "DefaultColorStringFormat" setting in the base TG config?

Beyond something like that, it's up to the consumer to specify how they want the output at the callsite.

ToString and Parse are round-trippable to the same underlying values (which is why that's what the tests for the Color type do). String representation is just markup.

dodexahedron commented 1 month ago

Closely related, though:

I was thinking it would be nice to expose HSL properties for on-the-fly conversion to make that kind of use easier anyway, as well as a ColorF type (like System.Drawing has) and maybe even ColorHsl or something of that nature, all implicitly convertible between each other.

Well... Maybe not implicitly for all cases, since conversion between the various colorspaces isn't a one-to-one and onto function. Would have to be explicit, but still I'd provide those as casts for ease of use.

tznind commented 1 month ago

I don't mind ToString returning "Red". Only want a method to call that formats consistently. g is 'general' maybe i just need x instead for hex.

Either way the source code for 'g' does not describe this ambiguous behaviour:

// g format string and null formatProvider - Output as 24-bit hex according to invariant culture rules from R, G, and B

dodexahedron commented 1 month ago

I also don't think it would be terrible to provide an explicit alias like ToHexString() that returns the result of ToString with the proper format argument, just 'cuz. That kind of place is where I don't really mind adding convenience stuff, even though it's not strictly necessary.

Maybe a Terminal.Gui.Extensions library might be in order, for adding a bunch of utility extension methods like that to various types, if people want them, so as not to violate our design rule against that sort of thing in TG itself? 🤷‍♂️