TreeView sometimes causing unit test failure

[BDisp]

          This test `Terminal.Gui.ViewsTests.TreeViewTests.TestTreeViewColor` sometimes fail and we need to check why.

  Failed Terminal.Gui.ViewsTests.TreeViewTests.TestTreeViewColor [43 ms]
  Error Message:
   System.Exception : Unexpected color White,Black was used at row 0 and col 0 (indexes start at 0).  Color value was White,Black (expected colors were -1,0)
  Stack Trace:
     at Terminal.Gui.TestHelpers.AssertDriverColorsAre(String expectedLook, ConsoleDriver driver, Attribute[] expectedColors) in /home/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/TestHelpers.cs:line 336
   at Terminal.Gui.ViewsTests.TreeViewTests.TestTreeViewColor() in /home/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/Views/TreeViewTests.cs:line 868
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Originally posted by @BDisp in #2941 (comment)

[tig]

Re-opening because I just got this on v2_develop. #2945 does not seem to fix this:

[BDisp]

Not lately. But try this fix https://github.com/gui-cs/Terminal.Gui/pull/2980/files#diff-a5631f70603a598990f3a83a457520dd29063c484472252ce4c157d0fae57a6d.

[tig]

Murmur, muble, muble, grrrr, mumble, mumble. 😠

[tig]

@BDisp I know you spent a ton of time on this (and I just noticed you just tried again after typing all this up!!!).

I just noticed something and am currently investigating. Would love to know your thoughts.

In this test the comments say some things that I don't think are right:

	[Fact]
	public void Changing_One_Default_Reference_Also_Change_All_References_But_Not_A_Instance_Reference ()
	{
		// Make two local attributes, and grab Attribute.Default, which is a reference to a static.
		Attribute attr1 = Attribute.Default;
		Attribute attr2 = Attribute.Default;
		// Make one local attributes, and grab Attribute(), which is a reference to a singleton.
		Attribute attr3 = new Attribute (); // instance

		// Assert the starting state that is expected
		Assert.Equal (ColorName.White, attr1.Foreground.ColorName);
		Assert.Equal (ColorName.White, attr2.Foreground.ColorName);
		Assert.Equal (ColorName.White, Attribute.Default.Foreground.ColorName);
		Assert.Equal (ColorName.White, attr3.Foreground.ColorName);

		// Now set Foreground.ColorName to ColorName.Blue on one of our local attributes
		attr1.Foreground.ColorName = ColorName.Blue;

		// Assert the newly-expected case
		// The last two assertions will fail, because we have actually modified a singleton
		Assert.Equal (ColorName.Blue, attr1.Foreground.ColorName);
		Assert.Equal (ColorName.Blue, attr2.Foreground.ColorName);
		Assert.Equal (ColorName.Blue, Attribute.Default.Foreground.ColorName);
		Assert.Equal (ColorName.White, attr3.Foreground.ColorName);

		// Now set Foreground.ColorName to ColorName.Red on the singleton of our local attributes
		attr3.Foreground.ColorName = ColorName.Red;

		// Assert the newly-expected case
		// The assertions will not fail, because we have actually modified a singleton
		Assert.Equal (ColorName.Blue, attr1.Foreground.ColorName);
		Assert.Equal (ColorName.Blue, attr2.Foreground.ColorName);
		Assert.Equal (ColorName.Blue, Attribute.Default.Foreground.ColorName);
		Assert.Equal (ColorName.Red, attr3.Foreground.ColorName);

		// Now set Foreground.ColorName to ColorName.White on the static of our local attributes
		// This also avoids errors on others unit test when the default is changed
		Attribute.Default.Foreground.ColorName = ColorName.White;

		// Assert the newly-expected case
		// The assertions will not fail, because we have actually modified the static default reference
		Assert.Equal (ColorName.White, attr1.Foreground.ColorName);
		Assert.Equal (ColorName.White, attr2.Foreground.ColorName);
		Assert.Equal (ColorName.White, Attribute.Default.Foreground.ColorName);
		Assert.Equal (ColorName.Red, attr3.Foreground.ColorName);
	}

Specifically:

// Make one local attributes, and grab Attribute(), which is a reference to a singleton.
Attribute attr3 = new Attribute (); // instance

Here, attr3 is NOT a reference to a singleton. The class Attribute is NOT a singleton. So this whole test has me confused, so I set about trying to understand more deeply...

I have been able to reproduce the TreeView test bug with this:

[Fact]
public void Small_Repro_of_TreeViewTests_TestTreeViewColor ()
{
	Assert.Equal (Color.White, Attribute.Default.Foreground.ColorName);
	
	Attribute defaultAttribute = Attribute.Default;
	Assert.Equal (Color.White, defaultAttribute.Foreground.ColorName);
	Assert.True (Equals (defaultAttribute, Attribute.Default));

	defaultAttribute = new Attribute (Color.Red, Color.Green);
	Assert.Equal (Color.Red, defaultAttribute.Foreground.ColorName);
	Assert.Equal (Color.White, Attribute.Default.Foreground.ColorName);

	defaultAttribute = Attribute.Default;
	defaultAttribute.Foreground.ColorName = Color.Blue;
	Assert.Equal (Color.Blue, defaultAttribute.Foreground.ColorName);

	// This fails because `defaultAttribute.Foreground.ColorName = Color.Blue` changes
	// the internal state of the `static readonly Attribute Default` field.
	Assert.Equal (Color.White, Attribute.Default.Foreground.ColorName);
}

This fails because defaultAttribute.Foreground.ColorName = Color.Blue changes the internal state of the static readonly Attribute Default field.

The current impl of Attribute.Default:

[JsonConverter (typeof (AttributeJsonConverter))]
public readonly struct Attribute : IEquatable<Attribute> {
	/// <summary>
	/// Default empty attribute.
	/// </summary>
	public static readonly Attribute Default = new Attribute (Color.White, Color.Black);

With this I (I think it was me) made it so Attribute.Default is readonly, but this does not prevent its members from being changed if it is assigned to another Attrbute and then THAT Attribute is changed.

The Small_Repro_of_TreeViewTests_TestTreeViewColor above shows this.

A fix would be:

[JsonConverter (typeof (AttributeJsonConverter))]
public readonly struct Attribute : IEquatable<Attribute> {
	/// <summary>
	/// Default empty attribute.
	/// </summary>
	public static Attribute Default => new Attribute (Color.White, Color.Black);

This makes Attribute a static property that is dynamic. Same as this:

[JsonConverter (typeof (AttributeJsonConverter))]
public readonly struct Attribute : IEquatable<Attribute> {
	/// <summary>
	/// Default empty attribute.
	/// </summary>
	public static Attribute Default {
		get {
			return new Attribute (Color.White, Color.Black);
		}
	}

With this, any reference to Attribute.Default results in a new instance.

However, while I think we should make this change (and I'm confident it fixes the TreeView testcase), I still think we have a problem: The fact that I can do ANYATTRIBUTEREF.Foreground.ColorName = Color.Blue; is a source of gnarly, hard to find bugs!

Therefore, I propose we

• Change Attribute.Default to be a static dynamic property with only a geter.
• Change Color.ColorName to be get only. We're lucky in that the only code in the project that does someAttribute.ColorName = colorName are in unit tests! To convince myself I'm correct on this, I commented out all those tests that set ColorName and the TreeView case no longer fails.
• Change Color.R/G/B/A/Rgba/ to be get only.
• Change ColorScheme in the same way. I spent an hour yesterday on an issue where I had done var cs = colorScheme vs var cs = new ColorScheme(colorScheme). This one is gonna be harder, as there's tons of code that will break, I fear.

New PR soon!

[BDisp]

Here, attr3 is NOT a reference to a singleton. The class Attribute is NOT a singleton. So this whole test has me confused, so I set about trying to understand more deeply...

It's bad, it's an new instance sorry. Only the Default is a singleton because it's static.

Or like this:

[JsonConverter (typeof (AttributeJsonConverter))]
public readonly struct Attribute : IEquatable<Attribute> {
	/// <summary>
	/// Default empty attribute.
	/// </summary>
	public static Attribute Default {
		get => new Attribute (Color.White, Color.Black);
	}

I'll close the #3162, right?

[tig]

I'll close the #3162, right?

Sí.

[tig]

Fuuuuuuu*ck!

[tig]

I think it may be:

• Change ColorScheme in the same way. I spent an hour yesterday on an issue where I had done var cs = colorScheme vs var cs = new ColorScheme(colorScheme). This one is gonna be harder, as there's tons of code that will break, I fear. We'll do this in : Colors: ColorSchemes refactor - Should honor terminal color settings etc... #2381

[tig]

I have not seen this in 5 days! I declare victory.

99% sure it was FakeDriver not setting PlatformColor to -1 on Make.