Casting inconsistent between `Key` and `KeyCode`

[tznind]

Describe the bug

Key and KeyCode support equality e.g.

// Passes
Assert.Equal (KeyCode.A, Key.A);
Assert.Equal (KeyCode.F1, Key.F1);

However when casting is involved things behave inconsistently. The most noticeable case is non character values e.g. F1

// Fails
Assert.Equal ((uint)KeyCode.F1, (uint)Key.F1);

The int cast on Key is going into the 'char' explicit operator which returns 0

Assert.Equal() Failure: Values differ
Expected: 1048587
Actual:   0

But there is also inconsistent behaviour when it comes to lower case letters

// Pass
Assert.Equal (KeyCode.A, Key.A);
Assert.NotEqual (KeyCode.A, Key.A.WithShift);

// Fail
Assert.Equal ((uint)KeyCode.A, (uint)Key.A);
Assert.NotEqual ((uint)KeyCode.A, (uint)Key.A.WithShift);

I think the code is going down the char route in order to get to uint and I note that the docs describes this as 'lossy'.

	/// <summary>
	/// Explicitly cast <see cref="Key"/> to a <see langword="char"/>. The conversion is lossy. 
	/// </summary>
	/// <param name="kea"></param>
	public static explicit operator char (Key kea) => (char)kea.AsRune.Value;

Expected behavior
Casting to uint should either be impossible or should be consistent

[BDisp]

Another strange behavior is the fact I can't debug this unit test in VS2022 with the expected parameter as Key. If I change it to KeyCode it will allow debugging. Although with the Key type I can't debug it seems to pass all. This also happens with you?

	[[Theory]
	[InlineData ("A-Ctrl", KeyCode.A | KeyCode.CtrlMask)]
	[InlineData ("Alt-A-Ctrl", KeyCode.A | KeyCode.CtrlMask | KeyCode.AltMask)]
	public void TryParse_ShouldReturnTrue_WhenValidKey (string keyString, Key expected)
	{
		Key key;
		Assert.True (Key.TryParse (keyString, out key));
		Assert.Equal (((Key)expected).ToString (), key.ToString ());
	}

[BDisp]

The reason why this is happening is because the InlineData can't be used to pass a class to pa parameter. If we use [InlineData ("a", new Key (KeyCode.A))] will throw the "An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type UnitTests \gui-cs\Terminal.Gui\UnitTests\Input\KeyTests.cs".
To pass a class to a parameter we need to use a class test inheriting from a IEnumerable, IEnumerable<object [] and yield return new object []. To test we use like this:

	[Theory]
	[ClassData (typeof (MyTestClass))]
	public void MyTestClassMethod (...parameters) {
	...
	}

[BDisp]

But in my opinion we don't need to pass the parameter as Key and only pass as KeyCode parameter, because KeyCode can be casted to Key. The following unit test also pass and it's possible to debug too.

	[InlineData ("A-Ctrl", KeyCode.A | KeyCode.CtrlMask)]
	[InlineData ("Alt-A-Ctrl", KeyCode.A | KeyCode.CtrlMask | KeyCode.AltMask)]
	public void TryParse_ShouldReturnTrue_WhenValidKey (string keyString, KeyCode expected)
	{
		Assert.True (Key.TryParse (keyString, out Key key));
		Key expectedKey = (Key)expected;
		Assert.Equal (expectedKey.ToString (), key.ToString ());
	}

[BDisp]

	[InlineData (KeyCode.CharMask, '\0')]
	[InlineData (KeyCode.SpecialMask, '\0')]
	public void AsRune_ShouldReturnCorrectIntValue (KeyCode key, Rune expected)
	{
		var eventArgs = new Key (key);
		Assert.Equal (expected, eventArgs.AsRune);
	}

struct types cannot be passed as parameter if we want to debug, so it's better to pass the Rune parameter as uint and then cast it to Rune.

	[InlineData (KeyCode.CharMask, '\0')]
	[InlineData (KeyCode.SpecialMask, '\0')]
	public void AsRune_ShouldReturnCorrectIntValue (KeyCode key, uint expected)
	{
		var eventArgs = new Key (key);
		Rune expectedRune = (Rune)expected;
		Assert.Equal (expectedRune, eventArgs.AsRune);
	}

Or

	[InlineData (KeyCode.CharMask, '\0')]
	[InlineData (KeyCode.SpecialMask, '\0')]
	public void AsRune_ShouldReturnCorrectIntValue (KeyCode key, uint expected)
	{
		var eventArgs = new Key (key);
		Assert.Equal ((Rune)expected, eventArgs.AsRune);
	}

[tig]

Not 100% sure the char cast operator is correct. We should dig in.

However the issue BDisp raises I did explore. In my 3rd attempt at this I decided not having to update as many unit tests as possible was important. So I tried to craft the operators (and constructors) to balance that.

I think having the unit tests depend on "ToString" and "TryParse" is an ideal long term solution as it makes the tests the most readable.

Or, put another way: for test inlinedata params, use KeyCode or string, never Key.

[BDisp]

Not 100% sure the char cast operator is correct. We should dig in.

No it isn't because isn't considering code points grater than 0xFFFF which is the max value of the char type. I think we should consider uint or int.

However the issue BDisp raises I did explore. In my 3rd attempt at this I decided not having to update as many unit tests as possible was important. So I tried to craft the operators (and constructors) to balance that.

I already changed many tests with the KeyCode on the fix I'm working related to the VKPacketSimulator which there is improvements on this matter.

I think having the unit tests depend on "ToString" and "TryParse" is an ideal long term solution as it makes the tests the most readable.

You're absolute right.

Or, put another way: for test inlinedata params, use KeyCode or string, never Key.

You're also absolute right. 😃

[tznind]

No it isn't because isn't considering code points grater than 0xFFFF which is the max value of the char type. I think we should consider uint or int.

Solution may be as simple as having both char and uint cast operators? (assume that is allowed).

[BDisp]

Solution may be as simple as having both char and uint cast operators? (assume that is allowed).

Not really. Well this is really ambiguous, because the KeyCode.F1 has no representation as Rune, like the KeyChar from a ConsoleKeyInfo, which is more or less this:

KeyChar='\0'
Key=ConsoleKey.F1

As it's returning the AsRune.Value the value will always be equal to 0. For the KeyCode.A it's returning 97 which is the representation of 'a' character. The solution maybe the following:

public static explicit operator uint (Key kea) => (uint)kea.KeyCode;

Do you both agree?

[tznind]

That makes sense to me.

[BDisp]

Solution may be as simple as having both char and uint cast operators? (assume that is allowed).

Maybe we should avoid to cast to char although is allowed, because it could return a value which does not represent the real value of the Key, because KeyCode is a flagged enum handling high values to allow having ConsoleKeyInfo's KeyChar, ConsoleKey and modifiers in only one Key representation. Also it can be cast from uint to char easely.

[BDisp]

Even better is using (Rune)(uint)Key because we are supporting Rune and not only char.

[BDisp]

@tig what tis do? I also see this on other places. You are excluding the KeyCode.Space from the wch if lower than or equal to 'z' which is wrong and lands to unexpected results.

			else if (wch <= 'z') {
				k = (KeyCode)wch & ~KeyCode.Space;
			} 

[tig]

@tig what tis do? I also see this on other places. You are excluding the KeyCode.Space from the wch if lower than or equal to 'z' which is wrong and lands to unexpected results.

			else if (wch <= 'z') {

				k = (KeyCode)wch & ~KeyCode.Space;

			} 

Without context it's hard to say. It may be a leftover I missed.

[dodexahedron]

No it isn't because isn't considering code points grater than 0xFFFF which is the max value of the char type. I think we should consider uint or int.

Solution may be as simple as having both char and uint cast operators? (assume that is allowed).

This is the way. At least char, for sure.

TerminalGuiDesigner has 53 tests broken by the changes to Key no longer consistently working with casts from char, due to case changes.

Test code, for reference, since Terminal.Gui likely needs similar test cases, since this is largely a test of Terminal.Gui behavior:

    [Test]
    public void HandleKey_AddsExpectedCharactersToView_AsciiAlphanumeric<T>( [ValueSource( nameof( Get_HandleKey_AddsExpectedCharactersToView_AsciiAlphanumeric_TestViews ) )] T testView, [ValueSource( nameof( GetAsciiAlphanumerics ) )] char k )
        where T : View
    {

        KeyMap? keyMap = null;
        Assume.That( () => keyMap = new(), Throws.Nothing );
        Assume.That( keyMap, Is.Not.Null.And.TypeOf<KeyMap>() );

        KeyboardManager? mgr = null;
        Assert.That( () => mgr = new( keyMap! ), Throws.Nothing );
        Assert.That( mgr, Is.Not.Null.And.TypeOf<KeyboardManager>() );

        Assume.That( testView, Is.Not.Null.And.InstanceOf<T>() );

        Assume.That( mgr, Is.Not.Null.And.InstanceOf<KeyboardManager>() );

        Design d = Get10By10View();
        Assume.That( new AddViewOperation( testView, d, "testView" ).Do() );
        testView.SetFocus();

        Assert.That( () => mgr!.HandleKey( testView,  (Key)k), Throws.Nothing );

        Assert.That( testView.Text, Is.Not.Null );
        Assert.That( testView.Text[ ^1 ], Is.EqualTo( k ) );
    }

    private static IEnumerable<char> GetAsciiAlphanumerics( )
    {
        for ( char charValue = char.MinValue; charValue < char.MaxValue; charValue++ )
        {
            if ( char.IsAsciiLetterOrDigit( charValue ) )
            {
                yield return charValue;
            }
        }
    }

The last line of the test will fail on any character that would have required a .WithShift under the new design. Cast operators to handle the char conversions I think would fix that up. But this is unicode. Dealing with shift, especially with different keyboard layouts, I think might be kind of nightmarish and isn't portable. For example, where is $ on your keyboard? How is that represented in Key and KeyCode, now? And how would that translate to someone using a US-english standard 101-key QWERTY keyboard? AZERTY? DVORAK? French? UK English? I suspect the new handling may be way too burdened by runtime culture differences.