Fixes #2800 - Color picker (supporting hsl, hsv and rgb)

[tznind]

Here is an updated ColorPicker WIP that supports HSL, HSV and RGB.

Requires a lot of tidy up and tests but this is the outline. I will investigate integrating with #2900 which has lots of stuff on colors and fallback colors (e.g. Ansi).

I studied how it works in Paint.Net and saw the following:

• Hue bar never changes, it always shows full rainbow
• Saturation and Lightness are dependent on Hue and their counterpart. The bar shows what the color would change to if you clicked in that cell.

For RGB the value rendered on each line indicates 'the color that will result if moving the cursor to the position on the bar'. This is why the colors of G and B change as you adjust R.

I think it is a really nice behaviour but happy to take suggestions and/or discuss approach.

I've learned a lot about color models!

Paint.Net Color Picker

ColorPicker2

Fixes

• Fixes Revamp ColorPicker and Color Scenarios in UICatalog #2800

Proposed Changes/Todos

• When bar area is black triangle should turn grey so it is still visible
• Tests
• Scenario
• Events

Pull Request checklist:

• I've named my PR in the form of "Fixes #issue. Terse description."
• My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• My code follows the Terminal.Gui library design guidelines
• I ran dotnet test before commit
• I have made corresponding changes to the API documentation (using /// style comments)
• My changes generate no new warnings
• I have checked my code and corrected any poor grammar or misspellings
• I conducted basic QA to assure all features are working

[tig]

This looks glorious!

[dodexahedron]

I like.

One thing I had on my personal to-do list was to actually just cherry pick the code from that ColorHelper library that we actually consume, since it's also MIT license, to avoid a whole library dependency for just a few methods and supporting types to make it happen. That seems to me like a nice thing to do for users. Also saves a non-trivial chunk of bytes for the package as well as work for the trimmer, which was actually what prompted me to consider adding that work item.

It also has the (IMO) important impact of not coupling a reference to Terminal.Gui to a reference to that library. Why does that matter? Because, if we cause a third-party dependency to be pulled in, consumers implicitly get access to that library in their project. If we were to switch the dependency or implement something ourselves or anything else that affected that, it could break people's applications until and unless they then manually reference that library after such a change were made to TG. And if they're doing strong naming and such, that might make upgrades on their end harder, as well.

If you wanna go for it, feel free. Otherwise, I'll just keep it as a low-priority item to address during beta/pre-release of TG. All good either way.

Could do it as a git submodule, but I've never loved that workflow and it seems to steepen the learning curve for getting going with a project, making it harder for people who aren't black belts in git-kwon-do to grok.

So, my intent is to preserve the original namespaces and formatting of the copied code and everything and tell the analyzers to buzz off for that code, so it doesn't add any warnings to our build and doesn't get reformatted. The default build configurations would be set up to build it that way. As part of the configuration I'm adding in #3558, I was planning to make a sort of "fully loaded" design time configuration, which would basically exclude that code and pull in the actual package of the same tagged version as the copied code, instead, so anyone actually working on enhancing features of TG would have access to the full library dependency and everything it offers, to make it just a matter of then copying over whatever new stuff gets used, before merge time, for the default configurations.

I actually vaguely remember seeing a Roslyn package out there that automates that last part at build time, even. I'll look into that whenever I get to it.

ALTERNATIVELY, we could just configure the build to trim that dependency specifically and not include the actual transitive build dependency in the package. At least I'm pretty sure there's a combination of incantations in msbuild to make that happen. If not, oh well. The other ways do it, just with slightly more XML. 🤷‍♂️

[tznind]

You have a Dispose bug somewhere.

Can confirm this occurs when switching color models. Seems was missing Dispose call in DisposeOldViews.

Fixed and test added in e371e99

[tznind]

Bring back the old ColorPicker as ColorPicker16

Given we have ColorModel in style options we might be able to do it that way instead?

cp.Style.ColorModel = ColorModel.SixteenColors;
cp.Style.ShowName = true;
cp.ApplyStyleChanges ();

This would show something like:

Mock up in which the 16 console colors would show on bar and it would be max length 16

We can have ShowName also work in the context of true color

The values allowed in Name would be

• If 16 color mode the named console colors
• Any other ColorModel it is the w3c names

If a color is selected in true color mode that is not a named w3c color

• Text field shows -

We can improve usability by having the Names text field use AppendAutocomplete

What do you think?

[tznind]

I have started on the named color support in 2efd89f so have converted to draft, here is the ColorPickers scenario with names on:

I have to do a lot of work for consistency and tests.

[tig]

Bring back the old ColorPicker as ColorPicker16

Given we have ColorModel in style options we might be able to do it that way instead?

cp.Style.ColorModel = ColorModel.SixteenColors;
cp.Style.ShowName = true;
cp.ApplyStyleChanges ();

This would show something like: Mock up in which the 16 console colors would show on bar and it would be max length 16

We can have ShowName also work in the context of true color

The values allowed in Name would be

• If 16 color mode the named console colors
• Any other ColorModel it is the w3c names

If a color is selected in true color mode that is not a named w3c color

• Text field shows -

We can improve usability by having the Names text field use AppendAutocomplete

What do you think?

I really liked the ability to have the 16 colors be in an arbitrary grid. For 16 colors the name & hex are not needed.

[tznind]

Ok I will finish up the name stuff for w3c as I think that still has value as an optional style.

Then see if I can restore and rename the old color picker.

[BDisp]

Why not using both in the same view. The HSV could be hide to only show the 16 colors, if true color isn't supported. If a user only want a 16 colors he click on the desire color where the name and value can be showed. If a user want a true color the HSV is sowed above or below the 16 colors. It could be fine that way.

[tznind]

The style options don't really make sense if doing 16 colors in a dynamic grid. This is why I proposed 16 being a bar with the 16 colors as 1x1 rectangles.

But I'd we want to allow 2x2 etc and 2 row layout of colors then it's too different to make sense as the same view.

I think if we like the old view ux it's best to restore it seperate

[tig]

Shouldn't these be in a string resource. Not for loc reasons, but to minimize memory use when this class is instantiated?

I think the new Color("#xxxx") ends up as code, which is less concerning, but it would be more efficient if these were specified as ints and not strings?

[tig]

usage:

        string white = ColorStrings.GetW3CColorName (0xFFFFFF);
        if (ColorStrings.TryParseW3CColorName (white, out Color color))
        {
               Debug.Assert(0xFFFFF, color);
        }

/// <summary>
///     Provides a mapping between <see cref="Color"/> and the W3C standard color name strings.
/// </summary>
public static class ColorStrings
{
    private static readonly ResourceManager _resourceManager = new ResourceManager (typeof (Strings));

    /// <summary>
    ///     Gets the W3C standard string for <paramref name="color"/>.
    /// </summary>
    /// <param name="color">The color.</param>
    /// <returns><see langword="null"/> if there is no standard color name for the specified color.</returns>
    public static string? GetW3CColorName (Color color)
    {
        // Fetch the color name from the resource file
        return _resourceManager.GetString ($"#{color.R:X2}{color.G:X2}{color.B:X2}", CultureInfo.CurrentCulture);
    }

    /// <summary>
    ///     Parses <paramref name="name"/> and returns <paramref name="color"/> if name is a W3C standard named color.
    /// </summary>
    /// <param name="name">The name to parse.</param>
    /// <param name="color">If successful, the color.</param>
    /// <returns><see langword="true"/> if <paramref name="name"/> was parsed successfully.</returns>
    public static bool TryParseW3CColorName (string name, out Color color)
    {
        // Iterate through all resource entries to find the matching color name
        foreach (DictionaryEntry entry in _resourceManager.GetResourceSet (CultureInfo.CurrentCulture, true, true)!)
        {
            if (entry.Value is string colorName && colorName.Equals (name, StringComparison.OrdinalIgnoreCase))
            {
                // Parse the key to extract the color components
                string key = entry.Key.ToString () ?? string.Empty;
                if (key.StartsWith ("#") && key.Length == 7)
                {
                    if (int.TryParse (key.Substring (1, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int r) &&
                        int.TryParse (key.Substring (3, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int g) &&
                        int.TryParse (key.Substring (5, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int b))
                    {
                        color = new Color (r, g, b);
                        return true;
                    }
                }
            }
        }

        color = default;
        return false;
    }
}

Then, in `./Resources/Strings.resx":

[tig]

Submitted a PR with a potential impl.

[tznind]

@tig the side effect of moving color names to a shared resources file is that all other strings are considered colors e.g. 'Save as'

I have added a test to show this.

[Fact]
public void TestColorNames ()
{
    var colors = new W3CColors ();
    Assert.Contains ("Aquamarine", colors.GetColorNames ());
    // Currently fails
    Assert.DoesNotContain ("Save as",colors.GetColorNames ());
}

How do you want to handle that? should there be a seperate resource file for the colors or is it possible to label them somehow so to tell the difference?

[tig]

The bug is in here:

public static IEnumerable<string> GetW3CColorNames ()
    {
        foreach (DictionaryEntry entry in _resourceManager.GetResourceSet (CultureInfo.CurrentCulture, true, true)!)
        {
            if (entry.Value is string colorName)
            {
                yield return colorName;
            }
        }
    }
```

Should be

```cs
public static IEnumerable<string> GetW3CColorNames ()
    {
        foreach (DictionaryEntry entry in _resourceManager.GetResourceSet (CultureInfo.CurrentCulture, true, true)!)
        {
            if (entry.Value is string colorName && colorName.StartsWith(`#`))
            {
                yield return colorName;
            }
        }
    }
```

(or similar)