Color parsing and formatting improvements

[dodexahedron]

I don't even fully remember what brought me to it, but I noticed the regexes being used in Color parsing, and that raised performance concerns.

Color.TryParse does what it says, mostly, though it does miss some edge cases and allows out-of-bounds values, since it uses all signed integers.

I wrote up a quick XUnit test harness to show a simpler and more precise implementation of the rgb portion of it that is several times faster, and executes in constant time, via a combination of simple string manipulation and the C# 12 collection expression feature, which enables additional optimizations by the compiler over older ways of manipulating certain types of collections.

Here's the code. The relevant portion is what's between the stopwatch starts and stops.

First, it tries out the new method, asserting the expected values were properly parsed. Then it does it the old way with the regexes (but with one tweak, to enable it to handle rgba as well).

The new code would replace the two regex blocks at the bottom of the TryParse method.

public class ColorExperiments ( ITestOutputHelper testOut ) {

    [Theory]
    [MemberData ( nameof ( StringsForColorParser ) )]
    public void ColorParsingMethodComparison ( string input, int expectedValues )
    {
        Stopwatch sw = new Stopwatch ( );
        long newRegexTicks = 0;
        for ( int iteration = 0; iteration < 15000; iteration++ ) {
            sw.Start ( );
            if ( input.StartsWith ( "rgb(", StringComparison.InvariantCultureIgnoreCase ) && input.EndsWith ( ')' ) ) {
                string [] split = input.Split ( (char []) [',', '(', ')'], StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries );
                switch ( split ) {
                case ["rgb", var rString, var gString, var bString]:
                {
                    var r = byte.Parse ( rString );
                    var g = byte.Parse ( gString );
                    var b = byte.Parse ( bString );
                    sw.Stop ( );
                    Assert.Equal ( r, expectedValues );
                    Assert.Equal ( g, expectedValues );
                    Assert.Equal ( b, expectedValues );
                }
                    break;
                case ["rgb", var rString, var gString, var bString, var aString]:
                {
                    var r = byte.Parse ( rString );
                    var g = byte.Parse ( gString );
                    var b = byte.Parse ( bString );
                    var a = byte.Parse ( aString );
                    sw.Stop ( );
                    Assert.Equal ( r, expectedValues );
                    Assert.Equal ( g, expectedValues );
                    Assert.Equal ( b, expectedValues );
                    Assert.Equal ( a, expectedValues );
                }
                    break;
                case ["rgba", var rString, var gString, var bString]:
                {
                    var r = byte.Parse ( rString );
                    var g = byte.Parse ( gString );
                    var b = byte.Parse ( bString );
                    sw.Stop ( );
                    Assert.Equal ( r, expectedValues );
                    Assert.Equal ( g, expectedValues );
                    Assert.Equal ( b, expectedValues );
                }
                    break;
                case ["rgba", var rString, var gString, var bString, var aString]:
                {
                    var r = byte.Parse ( rString );
                    var g = byte.Parse ( gString );
                    var b = byte.Parse ( bString );
                    var a = byte.Parse ( aString );
                    sw.Stop ( );
                    Assert.Equal ( r, expectedValues );
                    Assert.Equal ( g, expectedValues );
                    Assert.Equal ( b, expectedValues );
                    Assert.Equal ( a, expectedValues );
                }
                    break;
                default:
                    sw.Stop ( );
                    Assert.Fail ( "Input string not in a valid rgb(#,#,#) format" );
                    break;
                }
                newRegexTicks += sw.ElapsedTicks;
            }
        }
        sw.Reset ( );

        long originalRegexTicks = 0;
        for ( int iteration = 0; iteration < 15000; iteration++ ) {
            sw.Start ( );
            var match = Regex.Match ( input, @"rgba?\((\d+),(\d+),(\d+),(\d+)\)" );
            if ( match.Success ) {
                var r = int.Parse ( match.Groups [ 1 ].Value );
                var g = int.Parse ( match.Groups [ 2 ].Value );
                var b = int.Parse ( match.Groups [ 3 ].Value );
                sw.Stop ( );
                originalRegexTicks += sw.ElapsedTicks;
                Assert.Equal ( r, expectedValues );
                Assert.Equal ( g, expectedValues );
                Assert.Equal ( b, expectedValues );
            }

            match = Regex.Match ( input, @"rgba?\((\d+),(\d+),(\d+),(\d+)\)" );
            if ( match.Success ) {
                var r = int.Parse ( match.Groups [ 1 ].Value );
                var g = int.Parse ( match.Groups [ 2 ].Value );
                var b = int.Parse ( match.Groups [ 3 ].Value );
                var a = int.Parse ( match.Groups [ 4 ].Value );
                sw.Stop ( );
                originalRegexTicks += sw.ElapsedTicks;
                Assert.Equal ( r, expectedValues );
                Assert.Equal ( g, expectedValues );
                Assert.Equal ( b, expectedValues );
                Assert.Equal ( a, expectedValues );
            }

            originalRegexTicks += sw.ElapsedTicks;
        }

        testOut.WriteLine ( $"Original: {originalRegexTicks.ToString ( )} | New: {newRegexTicks.ToString ( )}" );

    }

    /// <summary>
    /// Generates cases from 0 to 255 each, for rgb and rgba formats, each with 3 or 4 parameters, to handle buggy inputs.
    /// </summary>
    public static TheoryData<string, int> StringsForColorParser ( )
    {
        TheoryData<string, int> cases = new TheoryData<string, int> ( );
        for ( int i = 0; i < 256; i++ ) {
            cases.Add ( $"rgb({i:D},{i:D},{i:D})", i );
            cases.Add ( $"rgb({i:D},{i:D},{i:D},{i:D})", i );
            cases.Add ( $"rgba({i:D},{i:D},{i:D})", i );
            cases.Add ( $"rgba({i:D},{i:D},{i:D},{i:D})", i );
        }
        return cases;
    }
}

The new code also implicitly gains bounds restrictions by using bytes, which will limit the input range to [0,255] or the appropriate overflow exception will be thrown by the framework (to be caught in TryParse).

This code is between 2 and 12 times faster on the machine I ran it on than the regex code.

Other portions of the TryParse method can be similarly modified to consolidate things into a single switch statement, for the other formats (such as hex), which would be nice, and which would also implicitly gain some free potential performance from use of collection expressions and the reduced allocations made possible with those language features.

At minimum, some of the hex format code can be simplified and improved like so (this is just in one case, but the method for each is nearly identical):

var colorBytes = Convert.FromHexString ( text [ 1.. ] );
color = new Color (colorBytes[0], colorBytes[1], colorBytes[2]);

That gets it done in one allocation of a byte array, instead of all the individual substring calls, each of which is a new string allocation.

[dodexahedron]

I was also experimenting with having Color store its values in a Vector3 field, instead of the individual ints (the properties R, G, B, and A still return ints pulled from the Vector).

That has some excellent performance benefits, too, though I wasn't ready to include any of it here since I didn't finish with that before wanting to fire off this issue first.

It also has built-in highly-optimized facilities for things like the Euclidian distance calculation, and internally can use SIMD instructions when relevant with no effort on the code side.

Even with the addition of casts that necessitates before returning values via the properties (since it stores floats), it still ends up blowing the ints out of the water in initial partial testing I did.

Also, even without that modification, some use of passing the Color struct by reference resulted in some nice speedups for things like the FindClosestColor, which otherwise ends up causing a ton of copies. That stuff will be in a separate issue, though. Just wanted to mention while it was on my mind.

[tig]

Big fan of what you're on to here, @dodexahedron. My focus on the Color refactor was on usability and features and I explicitly avoided premature performance optimization. Plus I'm not smart enough. Loving being able to learn from you!

[tig]

Oh, and this is gonna be even more important in the future because:

• 457

• 2729

• 2381

In #2729 I'm starting to take notes on how ColorScheme can be refactored to be more useful and flexible.

Etc...

[BDisp]

I was also experimenting with having Color store its values in a Vector3 field, instead of the individual ints (the properties R, G, B, and A still return ints pulled from the Vector).

That probably will for sure improving the performance.

[dodexahedron]

I like going over things with fresh eyes to see where easy benefits can be realized with relatively low effort. It's one of the reasons I jumped in on tackling the unit tests for TGD. :)

Also, so y'all can keep on with the things you're working on, I'll go ahead and finish this up and put in a PR when it's ready.

For this issue, the scope will be limited to the TryParse method, unless something else is just too good to pass up.

I forked from v2_develop as of earlier today, so I was working on code from commit 7fe95cb9c7f70c28f53264ab7a6c640f684c6da2.

But, there are several PRs ahead of that, and some did touch Color.cs, so unless anyone has any concerns or other suggestions on which code to use as base, I think I'll branch from a6dbfe6ed5ab19ad18b82109d863b2597e2bae00 on @BDisp's v2_timefield-textchanging-fix_3166 branch, which appears to be the latest as of this moment.

[BDisp]

But that doesn't include the fix of the DateField but for the Color is fine.

[dodexahedron]

Yeah those two should be independent at least.

[dodexahedron]

Got back to working on this, and here's an update on what I've got:

I couldn't help myself and did go beyond the initial intention of just improving parsing, for performance enhancements, some formality for the Color type (various interface implementations), and for some feature expansion (mostly related to those interfaces).

First, the Color struct had an easy performance and memory benefit to be gained by making it the C# equivalent of a union, via explicit layout of fields, so it is now internally just 32 bits, accessible via properties that expose those 32 bits as an unsigned integer, an integer, and 4 individual bytes for the color components, meaning no unchecked or unsafe blocks required, and saving 12 bytes. The existing properties are all there, though the individual color components themselves are bytes instead of ints.

I also added implicit casts to and from Vector3, which enables hardware-accelerated math, such as for CalculateColorDistance, which is now done via Vector3.Distance. That makes FindClosestColor many times faster (a quick test in a loop showed it to be over 6x faster on average).

I also turned it into a readonly record struct, which comes with some nice implicit compiler-generated goodies, and implicitly takes care of IEquatable<Color> and the value equality operators.

I then implemented several interfaces, including ISpanParsable<Color> (which implicitly includes IParsable<Color>) and IUtf8SpanParsable<Color>, for a more formal implementation of Parse (which now exists), and TryParse.

I also expanded its ability to handle some extra text formats, both for parsing and formatting via ToString(), and implemented ISpanFormattable (which implicitly includes IFormattable) and IUtf8SpanFormattable.

These interfaces of course add the ability to use them, but, more importantly, they are minimal-allocation means of doing these things, even when a string is passed, since string has implicit treatment as Span<char>. The I*Formattable interfaces also turn ToString into potentially more efficient calls, depending on how ToString is called, as well as eliminates boxing of the integral values during the formatting, which happened the old way.

Parameterless ToString has the same output it had before, but now there is flexibility to ask for other output formats via a few defined format strings, as well as any custom format string a consumer wishes to give it, if desired.

All existing tests continue to pass, as do the thousands of new test cases I've already written. I have more to add for the various ToString cases, as I'm not yet finished with that.

The end goal, for formatting and parsing, is for any string representation explicitly defined for parsing to be supported as an output format, as well.

Something to note about Color, that was already true: it's not binary-portable between big endian and little endian machines, as it assumes the byte order of the integer representation is little endian. That's fine in the majority of cases, though ARM machines can be big endian if configured that way. I'd say we continue to ignore that and let that be something such consumers have to deal with on their own. Perhaps we should document that Terminal.Gui assumes little endian byte order, though, as a general note, somewhere?

[dodexahedron]

Oh, and this is gonna be even more important in the future because:

• Colors: Update Color System (TrueColor, Blink, Underline, etc...) for v2 (Master Issue) #457
• [V2] truecolor support #2729
• Colors: ColorSchemes refactor - Should honor terminal color settings etc... #2381

In #2729 I'm starting to take notes on how ColorScheme can be refactored to be more useful and flexible.

Etc...

I rebased on the current v2_develop branch as of a little while ago, since these changes would otherwise have been conflicts at merge time.

I hadn't touched the moved code anyway, so it didn't affect me.

I also have a new ColorParseException class derived from FormatException that I'm considering using in the final form, because it's enabling me to add some helpful details to various exceptions that can happen during parsing (which obviously only escape Parse, and not TryParse). If that class does end up being helpful enough by the time I'm done, I'll stick it in its own file. If it doesn't get much use in my implementation, I'll leave it out and just use a stock FormatException. Right now, it's looking like it'll end up being used, because of those extra details.

[tig]

Whoah. Rad.

So much to learn from here. Very grateful!

Will dive deep asap and review.

[dodexahedron]

I haven't pushed the commits yet. I will when it's closer to completion. Plus I didn't do my usual micro-commits, due to how coupled a lot of the changes are.

I'll put in a PR at that time.

[dodexahedron]

Another side note...

I'm trying to be as accommodating of the alpha channel as I can, without changing existing behavior, to ease any potential future attempt to respect the alpha channel, if someone ever decides to do so.

[dodexahedron]

One cool thing about implementing IFormattable formally is that it makes color formatting extensible, if someone wants to pass in a custom IFormatProvider, to handle formatting in whatever way they wish.

It's also a globalization-related change since, in any potentially relevant cases, it allows culture-specific formatting to be used and respected, at least to the degree that whatever I end up with can do, if a custom IFormatProvider is given. The weird upshot of that is that, if someone passes non-latin numeric characters, it would in theory be able to handle it out of the box. I'm sure that's supremely unlikely, but just a kinda nifty side note about what supporting those interfaces enables, for honestly not much actual effort on my part aside from being sure the right overloads are invoked in the right situations.

[dodexahedron]

Ok

So

After what looks like a mostly-complete implementation of the new stuff, I have one pre-existing test that is failing: RuneCellTests.ToString_Override

I began to debug it but abandoned quickly when I saw that it looks like it may not be complete.

That test makes a call to TextView.ToString, which has an unused variable and method call in it. Looks like most likely just a line that was meant to be removed but forgotten when the property that does what that unused method call returns was used instead.

But the property it calls is named in a concerning way: DebuggerDisplay.

I pretty much stopped analyzing at that point because I didn't want to get too distracted from what I'm doing.

So, two requests:

1. Can someone revisit that and make appropriate adjustments?
2. Can someone explain exactly what the intent of the RuneCellTests.ToString_Override test is and why it is coupled so tightly to TextView?

There may be an additional case necessary in the new implementation of Color, but I want to be sure that the unrelated code that results in failure is at least confidently trusted before I continue with that.

As a general note, tests that are that unrelated causing failures in each other points to a problem either with the code that is under test or the test itself. Usually, it means that the test itself is not actually testing a self-contained "unit of work" and is instead more of an integration test, which is fine in the general sense, as long as it's understood by the test-writer. But, it also quite often reveals a potential bug or code quality issue in the form of code from distinct units that is coupled in a way that may not be intended. Both of those situations are things which should ideally be avoided, when possible.

In short, any "unit" test that incidentally results in a Color being parsed or formatted should "trust" that the code owned by Color is "correct" (which is why mocking is a common practice). This is of course a general statement and isn't restricted or only relevant to the Color class itself. Each class should have a test harness that exhaustively or at least adequately proves that that class, in a vacuum, behaves as expected and documented, in all reasonably conceivable cases.

When tests start making assumptions about the output of other classes, it means that the code under test is now coupled to a specific behavior of some other code, which is fragile and subject to breakage at future points, even with seemingly benign changes in that other code or in the code that depends on it.

De-coupling things appropriately, where there is tight coupling at present, can and probably will represent additional initial effort, but results in significant savings due to avoidance of what is some pretty high-interest technical debt.

[dodexahedron]

I did fix that failure by adding a case that was missing that I haven't yet written explicit tests to cover (I'm getting to that shortly), but I still think that test and potentially at least parts of the RuneCell class should be re-examined.

[dodexahedron]

I'm not a fan of some of the operators on Color.

For example:

    public static bool operator == (Color left, ColorName right) => left.ColorName == right;

What does that do, exactly? Well, if I were a consumer, I would assume it is value equality between a given Color and a named color.

Well... Nope... It's fuzzy equality, because ColorName returns the result of FindClosestColor. That's really not intuitive at all, for an equality operator, nor is it discoverable without either looking at the source or documentation. And the XmlDoc on those operators gives no hint at all that it's a fuzzy match (they are all variants on Equality operator for <see cref="Color"/> and <see cref="Gui.ColorName"/> objects.).

I really don't think that's appropriate for an operator==, especially on any kind of value type. The intended behavior really should be in a method named for what it does.

If I'm a consumer and I ask if a given color is equal to a ColorName, my expectation is that the operator tells me that the color value I have is exactly equal in value to the known/configured value for that named color. If I have "#F00000", I don't expect a value equality operator to ever tell me that it is == to Red, which is "#FF0000", but that's what it will do. And, with some of the implicit conversion operators available, it can lead to even more unexpected behavior if one of those happens as part of the equality operator call.

And then the inequality operators that should be the inverse of those are even weirder and make less sense. F00000 isn't FF0000, so why should operator!= return false if comparing those two values, just because one is named? If they were two unnamed colors, != would return true. That breaks the transitive property.

I'm a fan of what that code does, because it's a useful piece of functionality, if wanting to try to alias to EGA colors or whatever named color pallet, but that really belongs in dedicated methods and not the operators.

Now... As far as Terminal.Gui itself goes, there is no reference to that operator (or other similarly odd fuzzy operators) except in the unit tests, so I'm strongly inclined to change it and the other similar operators to named instance methods, which also makes them a bit more discoverable, anyway. I just am not quite sure what to call them. The dotnet standard for boolean methods is to use an active verb from the perspective of this, such as IsClosestTo(ColorName other), but I don't know if I love that name, specifically. Any thoughts?

In general, equality operators for value equality should have these properties, as laid out in the C# Programming Guide:

In either case, and in both classes and structs, your implementation should follow the five guarantees of equivalence (for the following rules, assume that x, y and z are not null):

1. The reflexive property: x.Equals(x) returns true.
2. The symmetric property: x.Equals(y) returns the same value as y.Equals(x).
3. The transitive property: if (x.Equals(y) && y.Equals(z)) returns true, then x.Equals(z) returns true.
4. Successive invocations of x.Equals(y) return the same value as long as the objects referenced by x and y aren't modified.
5. Any non-null value isn't equal to null. However, x.Equals(y) throws an exception when x is null. That breaks rules 1 or 2, depending on the argument to Equals.

Additional problems can arise when a color value is equidistant from two named colors. In that case, we have to make an arbitrary decision of how to break the tie, but doing so requires additional code to guarantee the transitive property is guaranteed (which isn't easy or is at least highly arbitrary).

[dodexahedron]

Oh, another thought about the fuzzy equality operators.

They have a high potential to break the use of Colors in any kind of HashTable or derived type, such as Dictionary, because two Colors that are "equal" can have a wide range of different results from GetHashCode, which is a biiiiig no-no.

If any Equals method or operator== on a type returns true, GetHashCode for the two objects should be identical.

[BDisp]

But the property it calls is named in a concerning way: DebuggerDisplay.

I pretty much stopped analyzing at that point because I didn't want to get too distracted from what I'm doing.

So, two requests:

1. Can someone revisit that and make appropriate adjustments?
2. Can someone explain exactly what the intent of the RuneCellTests.ToString_Override test is and why it is coupled so tightly to TextView?

@dodexahedron please see te reason of the use of the DebuggerDisplay in follow link. Thanks.

https://github.com/gui-cs/Terminal.Gui/pull/2682#discussion_r1230287506

[dodexahedron]

Yep.

I'm familiar with the attribute. Thanks for linking that issue, for context. It is a long-standing bug of Visual Studio that the debugger doesn't always show ToString exactly as intended, and it's annoying that it's still an issue. 😅

But the attribute isn't what I'm interested in.

The code in question is:

    public override string ToString ()
    {
        var colorSchemeStr = ColorSchemeDebuggerDisplay ();
        return DebuggerDisplay;
    }

The first line does nothing, as its return value is not used, and the method is pure, so that line can just be removed.

The second line is just odd, for a few reasons.

• Why is the public ToString override using the same property? Wouldn't a consumer calling it want the actual value and not an escaped version of it?
• The extra indirection of having the property and the method it calls, which is only referenced by that property, is unnecessary. The extra method can just be in-lined.
• Usually, if a DebuggerDisplayAttribute is necessary, it is either self-contained or uses ToString's output, with modifications. It is atypical to have ToString delegating through a property intended for internal debugging purposes, and might result in a confusing stack trace if an exception were to be thrown by something.
  • Both of those options also enable easier optimization at build time, for release builds, as it can be made conditional if desired.

Below is a version of it all that is much shorter, gives you the debug output you want with the escaped characters, and lets ColorScheme and Attribute handle their own ToString duties, so they aren't as coupled and output is consistent with anywhere else Attribute.ToString is called.

Here's the entirety of the modified RuneCell class:

/// <summary>
/// Represents a single row/column within the <see cref="TextView"/>. Includes the glyph and the foreground/background
/// colors.
/// </summary>
[DebuggerDisplay ( "{nameof(ColorSchemeDebuggerDisplay)}" )]
public class RuneCell : IEquatable<RuneCell> {
    /// <summary>
    /// The glyph to draw.
    /// </summary>
    [JsonConverter ( typeof ( RuneJsonConverter ) )]
    public Rune Rune { get; set; }

    /// <summary>
    /// The <see cref="Terminal.Gui.ColorScheme"/> color sets to draw the glyph with.
    /// </summary>
    [JsonConverter ( typeof ( ColorSchemeJsonConverter ) )]
    public ColorScheme? ColorScheme { get; set; }

    /// <summary>Indicates whether the current object is equal to another object of the same type.</summary>
    /// <param name="other">An object to compare with this object.</param>
    /// <returns>
    /// <see langword="true"/> if the current object is equal to the <paramref name="other"/> parameter;
    /// otherwise, <see langword="false"/>.
    /// </returns>
    public bool Equals ( RuneCell? other ) => other != null &&
                                            Rune.Equals ( other.Rune ) &&
                                            ColorScheme == other.ColorScheme;

    /// <summary>Returns a string that represents the current object.</summary>
    /// <returns>A string that represents the current object.</returns>
    public override string ToString ( )
    {
        var colorSchemeStr = ColorScheme?.ToString ( ) ?? "null";
        return $"U+{Rune.Value:X4} '{Rune.ToString ( )}'; {colorSchemeStr}";
    }

    string ColorSchemeDebuggerDisplay => ToString ( ); // Yes, this really does it. Visual Studio is just broken.
}

Here's the only addition that needs to be made to the ColorScheme class for that:

    /// <inheritdoc />
    public override string ToString ( ) => $"Normal: {Normal}; Focus: {Focus}; HotNormal: {HotNormal}; HotFocus: {HotFocus}; Disabled: {Disabled}";

And the only change to the test to match the output is:

            Assert.Equal ("U+0061 'a'; Normal: [Red,Red]; Focus: [White,Black]; HotNormal: [White,Black]; HotFocus: [White,Black]; Disabled: [White,Black]", rc2.ToString ());

Attribute brackets its output, so that's all that changed.

It's 16 lines shorter, overall, including the addition to ColorScheme, and performs a lot fewer string allocations (there was a bunch of concatenation before, and the "null" string literal allocation on every call, which usually just gets thrown away).

But it also now isn't inverted, as the property intended to make Visual Studio work properly is now slaved to ToString, and not the other way around.

It still outputs the escaped version for ToString, which I'm not a fan of, but I just didn't bother with that.

[dodexahedron]

@tig I went ahead and pushed the current state of the code (still work in progress) so you can have a look at where I'm going with it:

https://github.com/dodexahedron/Terminal.Gui/tree/v2_Color_TryParse_improvements_3170

I tried to break up a bunch of commits to show changes a little more progressively, but they're not great and I doubt many of the intermediate commits even build - it's just for review purposes.

The current commit efed19d75a6f069c7d1026ea41b33aa0684523ff builds and passes all unit tests, which includes a fair amount of new ones (but still plenty need to be made, which I'll get to). There are over 2000 test cases in ColorTests, but they should complete in less than a second (I'm seeing all 2042 of them complete in 0.1-0.2 seconds).

While this does do everything the old one did and more, it is still a work in progress, so I'm not ready to make a PR for it just yet.

[dodexahedron]

Oh and a note about the unit test project.

I found a really handy extension library for XUnit that gives it some similar combinatorial test case generation capabilities as NUnit has, and have been making use of that for tests I wrote from that point forward.

[tig]

As a general note, tests that are that unrelated causing failures in each other points to a problem either with the code that is under test or the test itself. Usually, it means that the test itself is not actually testing a self-contained "unit of work" and is instead more of an integration test, which is fine in the general sense, as long as it's understood by the test-writer. But, it also quite often reveals a potential bug or code quality issue in the form of code from distinct units that is coupled in a way that may not be intended. Both of those situations are things which should ideally be avoided, when possible.

In short, any "unit" test that incidentally results in a Color being parsed or formatted should "trust" that the code owned by Color is "correct" (which is why mocking is a common practice). This is of course a general statement and isn't restricted or only relevant to the Color class itself. Each class should have a test harness that exhaustively or at least adequately proves that that class, in a vacuum, behaves as expected and documented, in all reasonably conceivable cases.

When tests start making assumptions about the output of other classes, it means that the code under test is now coupled to a specific behavior of some other code, which is fragile and subject to breakage at future points, even with seemingly benign changes in that other code or in the code that depends on it.

De-coupling things appropriately, where there is tight coupling at present, can and probably will represent additional initial effort, but results in significant savings due to avoidance of what is some pretty high-interest technical debt.

I 1000% agree.

When I took over this project there were almost no unit tests. I pushed hard for all contributors to build them and a ton of folks stepped up. Sadly, none of us had really built unit tests ourselves at scale.

Additionally a lot of core code was much more coupled previously. This meant the only way to test something was to use AutoInitShutdown and compare the results of the driver's contents.

As a result, while we got good code coverage, many of the tests exhibit the bad behaviors you describe.

I've been working really hard on cleaning this up peace meal and have made some progress. I've also been much stricter in code reviews. Application is almost completely decoupled now, and View is THIS close to being able to be tested 100% without a driver being initialized. I caused @BDisp and @tzind a world of merge-hell in some of my bigger PRs in the last year.... a big reasons for all the changes was to further decouple things like this. We are mostly there now!

Thank you for highlighting it. Please feel free to open issues for any unit tests you think are egregious in this regard.

[tig]

I'm aligned with your analysis of ==. I don't recall if it was already there when I did my refactor if I added because I thought it would be useful...without really thinking it through.

No offense taken if you nuke it in this PR. I appreciate your clear articulation of the issues.

[tig]

@dodexahedron, without adding burden to an already epic endeavor, can I suggest you consider this:

https://spectreconsole.net/appendix/colors#:~:text=Standard%20colors%20%20%20%20%23%20%20,%20%20DarkYellow%20%2058%20more%20rows%20

I love what they're doing there w.r.t. supporting markup.

When we get to these:

• 17

• 1097

We will want similar capabilites.

At the minimum, as you think through your work her can you:

1) Ensure it paves a path for addressing those two issues? 2) Consider how we could actually USE code from Spectre vs. being arbitrarily different?

[dodexahedron]

I'm aligned with your analysis of ==. I don't recall if it was already there when I did my refactor if I added because I thought it would be useful...without really thinking it through.

No offense taken if you nuke it in this PR. I appreciate your clear articulation of the issues.

Glad my intention is taken as...well...intended! 😅

I certainly never mean to disparage or anything and my analyses are intended to just by dry, technical, and impersonal (though I know sometimes it's hard not to have some personal attachment to something we wrote or designed or whatever!). You should see some reviews I've done of some of my own old code in the past! Man, that guy had some silly ideas.

This PR likely will end up being essentially unreadable as a pure merge, which is why I tried to at least do some post-hoc commits to try to show things in a more digestible way. I was making rapid significant changes during the peak of the work, so making commits in real time would have resulted in a lot of noise.

I think from here I should probably be able to go back to a more normal (for me, anyway, haha) commit strategy, since the bulk of the implementation should be mostly set now.

I'll run the format rules now, though, before I resume work, because things are kinda scattered.

Oh! That reminds me of another handy little thing I added in one commit that's in the code as currently pushed to that branch....

The UnitTests/.filenesting.json file I added in bc5218 should be automatically picked up by VS in the UnitTests project. What that does is makes any .cs file in the same folder that has the same base name nest underneath the file with the base name, like how in a winforms or WPF project the codebehind files nest under the designer files.

I did that because I'm trying to keep things organized as I add a bunch of test methods, and breaking them into partial classes in files like ColorTests.Constructors.cs, ColorTests.Operators.cs, etc, which all nest in VS under ColorTests.cs, makes them much more manageable, and also helps keep noise down with changes such as reorganizing code. It also makes the UI less sluggish since there are smaller files loaded up at a time.

I'm happy to re-consolidate them after I'm done, if you like, but I tend to find the file nesting rules make a nice compromise that keeps things tidy while still allowing physical separation of logical units. 🤷‍♂️

[dodexahedron]

When I took over this project there were almost no unit tests. I pushed hard for all contributors to build them and a ton of folks stepped up. Sadly, none of us had really built unit tests ourselves at scale.

I'm glad that there is what there is and that you've done what you've done!

Miguel created one hell of a nifty and useful library, and I'm glad it lives on and that the current stewards are as willing to take a wrecking ball to things and modernize it as y'all are. :)

Additionally a lot of core code was much more coupled previously. This meant the only way to test something was to use AutoInitShutdown and compare the results of the driver's contents.

As a result, while we got good code coverage, many of the tests exhibit the bad behaviors you describe.

Yeah. Sometimes I'm pretty sure I can tell when something is an inherited legacy that's just being tolerated, because things just look different, ya know? And then it's also clear when the goal was to "get it done," which I toooootally feel your pain with haha. But hey - they're better than the 0 that was there before!

One big thing I know I probably keep repeating way too much is that, for most tests, smaller but more exhaustive tests are generally more helpful than longer tests that do a lot. After all, unit tests are supposed to be tests of units of work. :) To that end, the use of parameterized tests (which have to be marked Theory for XUnit) that prove the given unit actually behaves as expected for a range of inputs is so valuable I can't repeat it enough. 😅

As I'm working on Color, I'm also re-working most of the existing tests. Some are just getting unrolled into parameterized tests, some are getting expanded, some are getting re-worked, some are getting removed, and some are getting replaced, as appropriate - a lot of the same sort of work I've been doing for TGD, just XUnit-flavored over here.

That said, a couple questions for ya:

• Would you mind terribly if I included NUnit as well, if or when something would be significantly better or easier? That comes like 1/4 from personal preference and majority just from the fact that it's way more robust and expressive. No worries if you'd rather I not.
• Do you have a naming convention you're married to for test methods you'd like me to stick to? I try to be pretty descriptive with them, and thus far I've been mostly following along with what existing tests look like, with the exception of I usually don't also include the name of the class itself in the test method, since the test fixture already has that.
• Do you mind if, when it is important to do so, that I make something that was private be internal, for test purposes? I think that situation should typically be pretty rare, but just throwing it out there.

I've been working really hard on cleaning this up peace meal and have made some progress. I've also been much stricter in code reviews. Application is almost completely decoupled now, and View is THIS close to being able to be tested 100% without a driver being initialized. I caused @BDisp and @Tzind a world of merge-hell in some of my bigger PRs in the last year.... a big reasons for all the changes was to further decouple things like this. We are mostly there now!

Yeah I'm loving a lot of this kind of restructuring, as there is definitely plenty of old technical debt that needs to be paid down, and this is the perfect time to do it! Part of the reason I'm putting my effort in over here, right now, is that some of this stuff being in flux directly impacts what I was helping out with over at TGD as well, so might as well work on the core now instead of having to refactor multiple times.

Thank you for highlighting it. Please feel free to open issues for any unit tests you think are egregious in this regard.

For now, I'm still keeping this as restricted to Color, though I have been making note of other things I'd like to address once this is finished. I see plenty of other places that could benefit from some similar work, and should be majority non-breaking.

As you can probably see from what I've done with Color so far, I'm a big fan of flexibility through implementation of common and relevant interfaces, for multiple reasons. For one, the additional functionality is certainly nice to have. But, more importantly, it helps enforce a degree of standardization with other code and libraries - especially the BCL - as well as within this project itself. A pair of biggies that I think should be implemented in a lot of places are the IFormattable and IParsable interfaces and their Span-based derivatives. After all, this is a library for working with textual interfaces, so formal and consistent text parsing and formatting seems like a good baseline to promise to consumers.

[tig]

I dig the nesting idea. Keep it in and we'll see how it works!

[dodexahedron]

@dodexahedron, without adding burden to an already epic endeavor, can I suggest you consider this:

https://spectreconsole.net/appendix/colors#:~:text=Standard%20colors%20%20%20%20%23%20%20,%20%20DarkYellow%20%2058%20more%20rows%20

I love what they're doing there w.r.t. supporting markup.

When we get to these:

• Control: Attributed Label #17
• Leverage ANSI escape sequences as the mechanism for rich formatting w/in Terminal.Gui apps #1097

We will want similar capabilites.

At the minimum, as you think through your work her can you:

1. Ensure it paves a path for addressing those two issues?
2. Consider how we could actually USE code from Spectre vs. being arbitrarily different?

I've been thinking that using a limited form of XAML as markup would be wonderful, from a consumer perspective. Doesn't have to be nearly as comprehensive, given that this is a TUI, among other things, but it's a well-established markup for UI design and having something familiar could help adoption too.

As for the two mentioned issues, I've been noticing places where such things would be easier with some relatively minor but still somewhat breaking changes, so it's encouraging to hear that I'm not alone in wanting that sort of thing.

I think, though, that some things which can be layered on might do us well to eventually say "ok, that's for VNext," so we can get the already wonderfully improved V2 out and start getting feedback on it and learning whatever lessons we learn from whatever form it is in when we call it shippable. 😅

Things like more complete ANSI support and such probably fit into that category, in my opinion, but doing things in a way that makes those later additions easier is for sure an ideal I am completely on board with.

[dodexahedron]

I dig the nesting idea. Keep it in and we'll see how it works!

A nice thing about it is it's completely optional and you can toggle the behavior on and off right in the solution explorer. The UX for actually making and sharing the rules leaves something to be desired, and the syntax is anything but intuitive, but that's something that typically is set and forget, so that's no big deal to me.

[tig]

As I'm working on Color, I'm also re-working most of the existing tests. Some are just getting unrolled into parameterized tests, some are getting expanded, some are getting re-worked, some are getting removed, and some are getting replaced, as appropriate - a lot of the same sort of work I've been doing for TGD, just XUnit-flavored over here.

Thank you. I strongly suspect the work you are doing will serve as a great "how to do it RIGHT" for everyone else (including me). Thank you. Thank you.

• Would you mind terribly if I included NUnit as well, if or when something would be significantly better or easier? That comes like 1/4 from personal preference and majority just from the fact that it's way more robust and expressive. No worries if you'd rather I not.

I have no real love of xUnit. I used NUnit in another project and it was fine too. I'd like @BDisp and @tznind to chime in on this. I'm NOT a fan of using TWO test frameworks if we can avoid it. So, if you really think NUnit is signficantly better, and it makes your life significantly better, I would support a transition. But @BDisp and @tznind have to fully support this too because it will be a LOT of work.

• Do you have a naming convention you're married to for test methods you'd like me to stick to? I try to be pretty descriptive with them, and thus far I've been mostly following along with what existing tests look like, with the exception of I usually don't also include the name of the class itself in the test method, since the test fixture already has that.

• Be clear, concise, and complete. Clear means human readable and descriptive of the theory. Concise means terse (no class-name). Complete means no surprises pop when you actually look at the test code.
• Use underscores where you'd use a space. Use camel-case for names of classes/methods/properties that are camel-cased.

What else?

• Do you mind if, when it is important to do so, that I make something that was private be internal, for test purposes? I think that situation should typically be pretty rare, but just throwing it out there.

I actually encourage it. However, please add XML docs to any such element and have the docs say why it's internal (e.g. "This method is internal to support unit testing.). I wish there was a way of declaring a member "Make this internal for unit testing, but for god's sake don't let it be used anywhere else but this class."

[tig]

Oh yeah. See this: https://github.com/gui-cs/Terminal.Gui/wiki/Testing

Would be wonderful if someone (😉) updated that.

[dodexahedron]

As I'm working on Color, I'm also re-working most of the existing tests. Some are just getting unrolled into parameterized tests, some are getting expanded, some are getting re-worked, some are getting removed, and some are getting replaced, as appropriate - a lot of the same sort of work I've been doing for TGD, just XUnit-flavored over here.

Thank you. I strongly suspect the work you are doing will serve as a great "how to do it RIGHT" for everyone else (including me). Thank you. Thank you.

• Would you mind terribly if I included NUnit as well, if or when something would be significantly better or easier? That comes like 1/4 from personal preference and majority just from the fact that it's way more robust and expressive. No worries if you'd rather I not.

I have no real love of xUnit. I used NUnit in another project and it was fine too. I'd like @BDisp and @tznind to chime in on this. I'm NOT a fan of using TWO test frameworks if we can avoid it. So, if you really think NUnit is signficantly better, and it makes your life significantly better, I would support a transition. But @BDisp and @tznind have to fully support this too because it will be a LOT of work.

• Do you have a naming convention you're married to for test methods you'd like me to stick to? I try to be pretty descriptive with them, and thus far I've been mostly following along with what existing tests look like, with the exception of I usually don't also include the name of the class itself in the test method, since the test fixture already has that.

• Be clear, concise, and complete. Clear means human readable and descriptive of the theory. Concise means terse (no class-name). Complete means no surprises pop when you actually look at the test code.
• Use underscores where you'd use a space. Use camel-case for names of classes/methods/properties that are camel-cased.

What else?

• Do you mind if, when it is important to do so, that I make something that was private be internal, for test purposes? I think that situation should typically be pretty rare, but just throwing it out there.

I actually encourage it. However, please add XML docs to any such element and have the docs say why it's internal (e.g. "This method is internal to support unit testing.). I wish there was a way of declaring a member "Make this internal for unit testing, but for god's sake don't let it be used anywhere else but this class."

All sounds good to me and are pretty much how I operate anyway, so yay.

As for the test framework, it's not terribly important to me. TGD uses NUnit, and part of the work I've been doing there is actually updating the code from NUnit 2-era assertions to NUnit 4, which is current.

There are various libraries meant to work with multiple frameworks which can add in nice features without changing it all. One example would be FluentAssertions which basically just let's you use fluent-style method chains to make more expressive assertion statements, which is a similar idea to what NUnit has natively, but even takes the concept further and is of course an optional add-on (thus non-breaking to existing tests). And then libraries like the one I used for those combinatorial tests also exist in various forms. That one was pretty commonly.mentioned and recommended around the net, which is why I picked that one. That and it's an active project, which is also good.

In the end, a test harness is just code, and the framework is just syntactic sugar to reduce boilerplate.

However (and keep in mind I'm not pushing for a change, I'm just delivering information)....

Here are some key features native to NUnit that are just damn nice:

• Test ordering by simple Order attributes. Useful when you want to ensure that more basic, common core code is tested first, so time isn't wasted running a bunch of tests that will ultimately fail because some key dependency is broken.
• Pre-conditions using the Assume statement (rather than Assert. These are for the setup phase of a test fixture or test method, when you want to guarantee or prove a specific set of starting conditions for the actual test to be considered valid. Failing these gives the test case an Indeterminate result, which is more informative that something other than the code under test is causing the problem.
• More types of assertions built-in
• Parallel test execution with control granularity all the way down to the individual generated cases if parameterized tests.
• Combinatorial, pairwise, and sequential options for generation of test cases for parameterized tests. Sequential is handy and really cuts down on the need to write test case generator methods. The other two exist in a limited form in the extension I pulled in for xUnit.
• Much much much much MUCH better documentation. My god, the xUnit "documentation" is sorely lacking.
• Automatic expansion of enum parameters to a test case for each defined enum value, via this: [Values] (yep that's literally it), and that on top of the other rich parameterization goodness.
• Nice informational attributes for indicating what type or method a given test fixture or test method is testing. This is handy for informational purposes and some tools can also make use of it.
• Attributes to control which test methods or test fixtures are selected by the test runner based on the platform of the machine running the test (so you can test platform-specific code without messy conditional compilation or extra code in tests to handle that stuff)
• Threading control, such as forcing a test or fixture to run on a single thread, such as when testing certain things around statics.
• Tons more, but I'm feeling self-conscious like I'm advertising for it or something lol. Just have a look at the UnitTests project in TGD. I make use of everything I've mentioned here all over the place and more. 😅

Though one thing I will say that is intended somewhat as a directed point in support of switching: It's mostly a Find/Replace job to switch, as everything in the existing tests is there with the same syntax but just different attribute names.

Haha but enough about that. I'm gonna get back to work on Color and its tests. 😅

(This message clearly brought to you by NUnit.) 😆

[dodexahedron]

Oh yeah. See this: https://github.com/gui-cs/Terminal.Gui/wiki/Testing

Would be wonderful if someone (😉) updated that.

Ha.

Noted.

[dodexahedron]

Oh. And another quick off-topic but semi relevant thing:

Do you have a more updated dotsettings for resharper? The one that was there before was based on a much older language version and some analyzers and refactorings aren't working. I fixed one by adding a missing keyword (required) to the access modifier list, because it swore up and down it was not cool until I did so. 😅

And some of the newer language constructs confuse some of the older styling rules in it. I've been trying to stick at least mostly close to what I see as existing styles, but the darn adaptive styling has slowly turned into something closer to a hybrid of style guide recommendations, some of my preferences that somehow bled in, and some of the styles that were already there.

I know with virtual formatting and such that it's not really THAT big a deal these days, but if you do have something newer or more defined, I'd love to use it. But yeah - I'd like to be as minimally disruptive there as possible without having to fight the machine.

[dodexahedron]

I went ahead and pushed another set of commits with some basic type checks/change control tests, and some expansion of some existing tests, if you want to see some additional uses of stuff. The tests themselves aren't terribly profound. It's not like it's a complex type, at its core haha.

[tig]

Oh. And another quick off-topic but semi relevant thing:

Do you have a more updated dotsettings for resharper? The one that was there before was based on a much older language version and some analyzers and refactorings aren't working. I fixed one by adding a missing keyword (required) to the access modifier list, because it swore up and down it was not cool until I did so. 😅

And some of the newer language constructs confuse some of the older styling rules in it. I've been trying to stick at least mostly close to what I see as existing styles, but the darn adaptive styling has slowly turned into something closer to a hybrid of style guide recommendations, some of my preferences that somehow bled in, and some of the styles that were already there.

I know with virtual formatting and such that it's not really THAT big a deal these days, but if you do have something newer or more defined, I'd love to use it. But yeah - I'd like to be as minimally disruptive there as possible without having to fight the machine.

Nope. Sorry.

[tig]

(This message clearly brought to you by NUnit.) 😆

See

• 3007

[dodexahedron]

Ok, getting back to work on this after taking a break over the weekend. :)

There's some reorganization around Color in the next few commits I'm about to make, which result in several files being touched, but the changes at the callsites are simple and should be easy to resolve if there happen to be any merge conflicts.

[dodexahedron]

Alrighty

PR coming in shortly.

The functionality is implemented, but I haven't fleshed out all the tests, yet.

I merged current v2_develop into it to check for conflicts or new test failures, and that came back all good, so it's "ready," if you want, but more tests will be coming, so I'd suggest holding off for now.

[dodexahedron]

PR #3204 is in

All that's really left is additional test writing, so, unless new tests smoke out any issues, changes to Color itself should be basically done for this.

[kbilsted]

Just out of curiosity, it looks really cool what you've come up with! Why is try-parse called so often to call for such advanced code compared to the initial regex? I'm sure you have some use cases.

Have you contemplated caching already parsed values as an alternative? That could boost performance even more.

[dodexahedron]

I'm working on a bunch of stuff, so I'm not immediately sure of what you're referring to.

Can you point out a line and I'll explain?

Totally possible that there's opportunity for more gains.

[kbilsted]

the strings that are color parsed

[dodexahedron]

As for caching....

Well, as far as actual strings go, the runtime does that already, as it will intern strings if it finds it advantageous to do so.

But, for the actual work of things like when a color is matched against names (by the distance check or otherwise) I could see a potential opportunity to squeeze out a little more, and actually had considered it when I was writing it up.

However, the effort required wasn't worth it to me in the moment, especially since the span stuff is already so darn fast.

BUT! Just speaking purely in terms of theory:

Colors are value types. Therefore, once a color exists, any other identical values of it are technically redundant. So, upon creation of a new color from a string, a dictionary entry mapping that string to a boxed Color might have some advantages, but would of course come at the cost of a small amount of memory, and the boxing/unboxing of the color.

The thing is that tracking them via a cache introduces reference semantics for them, so you'd have to be careful. And the cost of boxing/unboxing is pretty high and probably more than a couple of byte matches in a Span (that collection pattern matching stuff in the big switch statement is powerful stuff that dotnet does).

An alternative idea that might be a better compromise could be to have a dictionary of likely common values outside of the named colors, which can be checked first.

The thing is, any cache keyed on a string is going to result in checking the string for equality anyway, which is most of the actual time-consuming work the parser does. So, my prediction is it wouldn't actually make a tangible difference, or could actually result in worse performance.

[dodexahedron]

the strings that are color parsed

Gotcha. You posted as I was writing the above response, so I think that question is addressed there?

Key thing is that string comparisons are always going to happen, whether there's a cache or parsing is done in real-time, so there's already a sunk cost. But, the parser uses spans, which are significantly more performant than strings (it's kinda crazy how good they are in comparison), so any alternative is going to have to overcome that on top of anything else.

[dodexahedron]

As for string interning... We could explicitly TELL the runtime to intern color strings, as well, to guarantee that it happens once a unique color string is encountered.

For any of these, though, I think we might be getting into micro-optimizations that are not as valuable as spending the effort elsewhere, at least until RTM. 🤷‍♂️

As much as I like to optimize things, exercising the self-control to not take it to the extreme is sometimes hard. 😅

[kbilsted]

Performance is also relative to numbers. How many color parsing executions would you recon there be in a single application execution?

[dodexahedron]

It's a good question and something I've been thinking of when working on this and when looking at other value types like Pos.

It almost made me want to turn Color into a class, but it really is a good one to be a value type.

As far as actually parsing a raw string goes? I would think that the majority of that is done at startup, when reading configuration. Otherwise, in a compiled application, the compiler is going to work its magic on a lot of stuff, with the value types, where it can.

At runtime, we're mostly actually passing around an actual Color, so the parsing stuff shouldn't come up much after startup.

It would probably be a better use of time and effort to track down any place a color is created (including by implicit copy) and either fix that or be sure to stick to reference passing. In this current PR, I did already add a bunch of in modifiers in various function calls to avoid copying Color values on method calls. Making sure that's done everywhere it is possible to do so would be a good idea.

That said, I did make various constructors and methods in Color take in Color parameters, already, so the majority of those cases should already be covered implicitly because you don't have to specify in at the call site to get the benefit of that.

[dodexahedron]

With nothing specific in mind, I would say that, if there are places where we pass around a ColorName or other representation of a Color when we could just as easily pass around a reference to a Color, we should try to pass the Color instead. I did fix one or two of those, I think, and I don't know if there are other instances of that. But that could be an easy free gain of performance anywhere it may still happen.

[tig]

Performance is also relative to numbers. How many color parsing executions would you recon there be in a single application execution?

A ColorScheme has 20 Color objects. There are currently 5 ColorSchemes. That's 100.

There are currently 4 Themes defined in Configuration Manager. When it loads that's 4 x 100, today.

We know the current ColorScheme scheme (pun intended) is lacking and there will likely be ~2x more in v2 when we're done.

This is relevant for app load, and for use-cases where the theme is changed.