Casting inconsistent between `Key` and `KeyCode` #3071

tznind · 2023-12-23T05:22:49Z

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

The text was updated successfully, but these errors were encountered:

BDisp · 2023-12-23T10:18:07Z

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 · 2023-12-23T10:52:07Z

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 · 2023-12-23T13:05:53Z

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 · 2023-12-23T13:16:53Z

	[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 · 2023-12-23T15:24:11Z

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 · 2023-12-23T16:03:22Z

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 · 2023-12-23T17:08:53Z

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 · 2023-12-23T17:22:34Z

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 · 2023-12-23T17:30:21Z

That makes sense to me.

BDisp · 2023-12-23T17:44:35Z

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 · 2023-12-23T18:10:34Z

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

BDisp · 2023-12-23T23:20:56Z

@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 · 2023-12-23T23:26:58Z

@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 · 2023-12-24T01:00:09Z

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.

BDisp · 2023-12-24T01:13:21Z

@dodexahedron can you convert it to xunit please?. Thanks.
I would like to add it to the Terminal.Gui.

dodexahedron · 2023-12-24T01:23:37Z

It's TGD specific because it is testing the KeyMap and KeybooardManager, and involves other TGD-specific classes.

It's just for reference of how it breaks.

The last line of the test method and the line two above that are the most interesting for you. After having handled a key event, cast from a char, (third line from bottom), it then checks the last character in the view (last line). For anything requiring a shift (however that is defined) it will be broken.

dodexahedron · 2023-12-24T01:27:05Z

Basically, you probably want the helper method below it and then a proper test in Terminal.Gui would need to ensure the round trip from char to Key to char works without change, tested in multiple cultures and ui cultures.

In NUnit, the way that is set up results in a bunch of individual test cases getting created: one per character output by the helper method shown there, all repeated once per type being tested (it currently tests TextBox and Label, but that's not relevant in the context of this issue).

BDisp · 2023-12-24T01:32:37Z

Take a look on the https://github.com/gui-cs/Terminal.Gui/pull/3078/files#diff-b06483ffe0f3297a11416013982e6ceb777c6a35cbcfec41afe180eaa5b15132 where I have some unit tests that probe it's working better. Thanks.

dodexahedron · 2023-12-24T01:37:08Z

I would strongly suggest using whatever equivalent xUnit has to the generation of all of the possible test cases, or else just testing them all inside a single test method, if that's not possible. The possibilities for problematic input handling are vast, when considering different keyboard layouts and cultures, which make the concept of a letter differing solely by the shift key non-portable. For the test in the context of Terminal.Gui to be valid, it needs to at least handle all printable 8-bit characters and whitespace, along with control characters and function keys.

That single test method, for example, covers 124 cases in TGD, and we know exactly which ones fail because they are treated as individual cases. If I were to add additional cultures, it would multiply that 124 by the number of cultures tested. I am not familiar with xUnit, so I don't know what it has for that kind of thing, but input handling is so crucial that it needs to be provable across all elements of a reasonable range of inputs.

Note the helper method I showed only provides what the current culture's concept of alphanumeric characters are. If you ran that in a spanish locale, I imagine it would generate more cases than it does on mine, which are US english.

BDisp · 2023-12-24T01:44:44Z

It covers more than 124 cases and I think it would work with some cultures.

dodexahedron · 2023-12-24T02:05:38Z

I think you might misunderstand my meaning, there.

I mean to say they are not exhaustive tests. Each one spot-checks a few inputs, and the inputs checked are not consistent. The sample I showed covers all 62 alphanumeric characters in US english, across two possible View types that have a Text property. (thus 124). JUST for the case of round-tripping the char value from char to Key to char. 52 of those fail (26 for each View, with those 26 being every capital letter). It is therefore a proof that all of those characters round-trip as expected, which they used to, but now do not.

A specific set of behaviors should be defined, and then a test theory should test that behavior and ensure that it is consistent across the entire input domain or at least a reasonable subset of it (such as 8-bit characters, as mentioned before).

Non-exhaustive tests result in a lot of things slipping through the cracks (for example, I've discovered plenty of subtle things to fix in TGD as I've been formalizing the tests).

dodexahedron · 2023-12-24T02:09:09Z

I pointed out one example of a problematic test in #3078 , to provide an example and context for this discussion.

tznind · 2023-12-24T09:14:35Z

Why are we using our own types for this?

ConsoleKeyInfo is cumbersome, it lacks many keys and has ambiguous entries e.g. the Oem keys. There are environmental issues too, for example some deployments will see only 'virtual key' and the unicode char as input. Terminal.Gui does a lot of standardisation at the console driver level. I don't think changing to core types would improve the library user experience. Current issue just seems to be tied up with the use of shift/capital letters.

…

On Sun, 24 Dec 2023, 03:22 dodexahedron, ***@***.***> wrote: Also, it is important to note that caps lock also makes the "WithShift" behavior potentially odd. ConsoleKeyInfo already handles that. Here's the output from the same code as above but with caps lock on: Key: A | Modifiers: None | Unicode Value: 65(0x41) | Resulting Character: A Key: A | Modifiers: Shift | Unicode Value: 97(0x61) | Resulting Character: a Key: A | Modifiers: Alt | Unicode Value: 65(0x41) | Resulting Character: A Key: A | Modifiers: Alt, Control | Unicode Value: 0(0x0) | Resulting Character: Key: A | Modifiers: Alt, Shift, Control | Unicode Value: 0(0x0) | Resulting Character: Key: A | Modifiers: Alt, Shift | Unicode Value: 97(0x61) | Resulting Character: a — Reply to this email directly, view it on GitHub <#3071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHO3C5HSCM42GH4CP3INU5LYK6NYVAVCNFSM6AAAAABBARZAJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGQZDEOJRHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

BDisp · 2023-12-24T12:46:47Z

@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.

Here the same. It's limiting the caps lock to only characters lower or equal to Z. If caps lock is pressed then the shift key must be added to all characters.

			if (((keyInfo.Modifiers == ConsoleModifiers.Shift) ^ (keyInfoEx.CapsLock))) {
				if (keyInfo.KeyChar <= (uint)KeyCode.Z) {
					return (KeyCode)((uint)KeyCode.A + delta) | KeyCode.ShiftMask;
				}
			}

tig · 2023-12-24T13:58:59Z

If caps lock is pressed then the shift key must be added to all characters.

This is not correct.

On a US keyboard CapsLock does NOT apply to D1-D9, <, etc...

This may be different on different keyboards.

BDisp · 2023-12-24T14:06:20Z

If caps lock is pressed then the shift key must be added to all characters.

This is not correct.

CapsLock does NOT apply to D1-D9, <, etc...

You are right, my bad. But it's only handling characters from A to Z and there is no need to add the conditional if (keyInfo.KeyChar <= (uint)KeyCode.Z) because the KeyChar will never be 'a' if caps lock is enabled. Right?

tig · 2023-12-24T14:07:03Z

If caps lock is pressed then the shift key must be added to all characters.

This is not correct.
CapsLock does NOT apply to D1-D9, <, etc...

You are right, my bad. But it's only handling characters from A to Z and there is no need to add the conditional if (keyInfo.KeyChar <= (uint)KeyCode.Z) because the KeyChar will never be 'a' if caps lock is enabled. Right?

Yeah, i think so.

BDisp · 2023-12-24T14:42:34Z

This will get the same KeyChar e.g. Ç (199) will return Ç (199) or if it's Z (90) will return Z (90) anyway. I think you did want if (keyInfo.Modifiers == 0 && ((KeyCode)((uint)keyInfo.KeyChar) | KeyCode.Space) == 0) but that will change the real KeyChar and we don't want that.

			if (keyInfo.Modifiers == 0 && ((KeyCode)((uint)keyInfo.KeyChar) & KeyCode.Space) == 0) {
				return (KeyCode)((uint)keyInfo.KeyChar) & ~KeyCode.Space;
			}

tig · 2023-12-24T15:52:21Z

It occurred (and still does) to me that for Key, the KeyCode property could be immutable. I can't think of any case where after a Key object is created that KeyCode needs to be changed.

Thus, in addition to changing the helper "constants" (Key.X) to be get-only, we can make KeyCode "get; init".

This further reduces the chance of a static issue and makes the intent even more clear.

BDisp · 2023-12-24T16:10:33Z

Just stepping back for a moment, I think this is an important question to ask:

Why are we using our own types for this? This is a solved problem in the base class library of dotnet with standard use and representation, is it not? Why create an alternative that bucks the convention and creates the burden of testing and maintenance for it?

Well we really don't want and we aren't re-event the base class library. What we are doing is using only only Key to manage all the keys combinations at once. With ConsoleKeyInfo you have to check if KeyChar is what you want and if Modifiers is what you want and probably if the ConsoleKey is what you want. With the Key class we have all the information at once. e.g. To check if the A was pressed with the Ctrl key, using the ConsoleKeyInfo you have to check if KeyChar == 'A' and if Modifiers == ConsoleModifiers.Control. Using the Key class you only need to check if key == KeyCode.A | KeyCode.Ctrl. We are trying to be consistent with the dotnet library.

tig · 2023-12-24T16:16:09Z

A note on cast. I noticed in another pr from BDisp, he changed the api doc comment to say "the cast is never lossy".

This is not right.

Casting a Key to anything other than a Key is definitely lossy. For example whatever Key.Handled is set to is lost in the cast. This is why those operators are "explicit" and not "implicit".

BDisp · 2023-12-24T16:23:11Z

A note on cast. I noticed in another pr from BDisp, he changed the api doc comment to say "the cast is never lossy".

I only was referring about cast from Key to Rune. If the character is a surrogate pair and you cast to char then the "cast is lossy" because a char is 16 bits length and a surrogate pair is a 32 bits.

tznind · 2023-12-24T16:59:03Z

I can't think of any case where after a Key object is created that KeyCode needs to be changed.

Sometimes in TGD I change the key e.g. when Enter is pressed in a menu I add another menu item and change the KeyCode to Down so that when it finishes resolving, the new menu item is highlighted.

But I'll grant you its a pretty obscure use case and I currently make do with reflection so would be fine with immutableness. You can even call set on a property with no setter at all with this lovely hacky bit of reflection: https://stackoverflow.com/a/40917711/4824531

private void ChangeKeyTo(Key keystroke, Key newKey)
{
      Type t = typeof(Key);
      var p = t.GetProperty("KeyCode") ?? throw new Exception("Property somehow doesn't exist");
      p.SetValue(keystroke, newKey.KeyCode);
}

dodexahedron · 2023-12-24T20:43:28Z

If caps lock is pressed then the shift key must be added to all characters.

This is not correct.
CapsLock does NOT apply to D1-D9, <, etc...

You are right, my bad. But it's only handling characters from A to Z and there is no need to add the conditional if (keyInfo.KeyChar <= (uint)KeyCode.Z) because the KeyChar will never be 'a' if caps lock is enabled. Right?

If caps lock is on and shift is also pressed, a yields a.

dodexahedron · 2023-12-24T20:54:41Z

ConsoleKeyInfo is cumbersome, it lacks many keys and has ambiguous entries e.g. the Oem keys. There are environmental issues too, for example some deployments will see only 'virtual key' and the unicode char as input. Terminal.Gui does a lot of standardisation at the console driver level. I don't think changing to core types would improve the library user experience. Current issue just seems to be tied up with the use of shift/capital letters.

Coolio.

Then I think a good starting point to resolving this type of issue is to write a test that round-trips all possible raw keyboard inputs (at least that are representable by Key) to Key and back and ensures value equality of all fields/properties of the round-tripped input. Might smoke out other subtle things as well.

BDisp · 2023-12-24T21:09:35Z

If caps lock is on and shift is also pressed, a yields a.

You are absolute right and all drivers are doing the right behavior if you try it.

BDisp · 2023-12-24T21:29:14Z

Only the WindowsDriver handles caps lock and here where it's handled
https://github.com/BDisp/Terminal.Gui/blob/a938295eeb98950f7a65b4afb5377e1210c9d6a6/Terminal.Gui/ConsoleDrivers/WindowsDriver.cs#L1009-L1013. The others drivers the terminal already provide the right character.

tig · 2023-12-24T22:42:34Z

If lower case letters existed when computer keyboard standards were established you'd be right about correctness.

But they didn't. ASCII 65 == VK_A which is upper case 'A'. On non-us keyboards the keys between VK_A and VK_Z are not always A..Z. But they always result in a lower case char unless shift is pressed or caps lock is on.

We could choose to make KeyCode.A be KeyCode.a, but we already went down that path and it led to having BOTH KeyCode.A and KeyCode.a which just moved the complexity and masked it with more.

So... terminal.gui's low level encoding of keys (KeyCode) special -cases the A..Z keys as is well documented in the Key docs in current v2_develop branch (I just noticed the KeyCode docs are incorrect as they say A..Z mean shifted). The high-level abstraction of Key carries this forward with the Key.A..Z helper properties.

Users are free to use neither of these and just do new Key('a') or new Key('A') if they want.

tznind · 2023-12-24T23:07:19Z

There are limitations placed by console protocol. We don't have hardware access, only what the console terminal emulator sends. So for example there is no key down, key up sent by Linux terminals. If user pastes into terminal e.g. ctrl+shift+v - we don't see that either only a sequence of characters that were pasted. So Shift+A is the same as CapsLock A; is the same as pasting a capital A. We just have to do the best we can as a library to standardise the input to a common denominator across all the drivers and make it as easy to consume as possible for API users.

…

On Sun, 24 Dec 2023, 21:29 BDisp, ***@***.***> wrote: Only the WindowsDriver handles caps lock and here where it's handled https://github.com/BDisp/Terminal.Gui/blob/a938295eeb98950f7a65b4afb5377e1210c9d6a6/Terminal.Gui/ConsoleDrivers/WindowsDriver.cs#L1009-L1013. The others drivers the terminal already provide the right character. — Reply to this email directly, view it on GitHub <#3071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHO3C5EQWYBUEWVSUKQG76TYLCNDLAVCNFSM6AAAAABBARZAJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGU4TMNRRGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

BDisp · 2023-12-24T23:09:49Z

We are mapping the correct ConsoleKeyInfo into KeyCode. Remember that's A and a belong both to the ConsoleKey.A which is the upper case. The difference is that A have the shift modifier and the a don't. I admitted that my PR may have some incorrect code but the main code satisfy the needs.

BDisp · 2023-12-25T00:58:27Z

It seems you think we are changing the original keys from terminal and we are not. It's better present the bugs by showing trough a unit test.

dodexahedron · 2023-12-25T01:02:45Z

It's off-topic of the original issue, anyway, as it was only incidentally related.

I had a sample demonstrating what I was originally getting at, last night, but I'll have to re-create it. I don't think I'm on the right/same track as what I was dealing with at that point (or could have missed something then in the first place), so I think I'm creating unnecessary confusion at the moment. I'll hide the off-topic tangent here for now until I figure out WTH I was specifically hitting and file a separate issue, if it really is relevant. Apologies for the de-rail.

dodexahedron · 2023-12-25T03:15:52Z

Users are free to use neither of these and just do new Key('a') or new Key('A') if they want.

That constructor doesn't currently exist, though it would be better than the cast path from char to Key, if you ask me, since calling a constructor is a pretty intentional act.

I would propose perhaps a modified version of your thought from earlier, about making almost everything about a Key immutable actually:

Lots of ways to achieve it¹, but making them all init-only gets that desired immutability while still allowing users to mess with it in an object initializer if desired. KeyCode already is, as it turns out. I don't think it needs further restriction than that. Maybe also provide optional parameters in the constructors, in case someone wants to explicitly build the Key with, for example, certain modifiers set (which would also cover a use case @tznind mentioned)?

For example:

public Key( char c, bool shift = false, bool ctrl = false, bool alt = false )
// ...

I'm a fan of records for that purpose, personally, and that's a good place for a readonly record struct or a plain old record ↩

tig · 2023-12-25T15:06:55Z

Still a WIP (Merry Christmas!) but I hope this helps!

#3089

* Removed char->Key cast. Added Key(char) * Re-added char->key cast. Added unit tests * Fixed standard keys to always return new instead of being readonly * Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up * Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up * Adds string constructor to Key + tests. * Simplified Key json * Added string/Key cast operators.

…rrect (gui-cs#3089) * Removed char->Key cast. Added Key(char) * Re-added char->key cast. Added unit tests * Fixed standard keys to always return new instead of being readonly * Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up * Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up * Adds string constructor to Key + tests. * Simplified Key json * Added string/Key cast operators.

tznind added the v2 For discussions, issues, etc... relavant for v2 label Dec 23, 2023

dodexahedron mentioned this issue Dec 24, 2023

Compiling with Terminal.Gui 2.0.0-pre.243 gui-cs/TerminalGuiDesigner#268

Merged

5 tasks

dodexahedron mentioned this issue Dec 24, 2023

Cleans up key handling in drivers (was "fixes VkeyPacketSimulator is broken") #3078

Merged

6 tasks

This comment was marked as off-topic.

Sign in to view

tig mentioned this issue Dec 25, 2023

Fixes #3071 & #3079. Key cast and static props are not correct #3089

Merged

14 tasks

tig closed this as completed Jan 4, 2024

Casting inconsistent between Key and KeyCode #3071

Casting inconsistent between Key and KeyCode #3071

Comments

tznind commented Dec 23, 2023

BDisp commented Dec 23, 2023

BDisp commented Dec 23, 2023 • edited Loading

BDisp commented Dec 23, 2023

BDisp commented Dec 23, 2023 • edited Loading

tig commented Dec 23, 2023 • edited Loading

BDisp commented Dec 23, 2023

tznind commented Dec 23, 2023

BDisp commented Dec 23, 2023

tznind commented Dec 23, 2023

BDisp commented Dec 23, 2023

BDisp commented Dec 23, 2023

BDisp commented Dec 23, 2023

tig commented Dec 23, 2023

dodexahedron commented Dec 24, 2023 • edited Loading

BDisp commented Dec 24, 2023 • edited Loading

dodexahedron commented Dec 24, 2023

dodexahedron commented Dec 24, 2023 • edited Loading

BDisp commented Dec 24, 2023

dodexahedron commented Dec 24, 2023 • edited Loading

BDisp commented Dec 24, 2023

dodexahedron commented Dec 24, 2023

dodexahedron commented Dec 24, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

tznind commented Dec 24, 2023 via email

BDisp commented Dec 24, 2023

tig commented Dec 24, 2023 • edited Loading

BDisp commented Dec 24, 2023

tig commented Dec 24, 2023

BDisp commented Dec 24, 2023

tig commented Dec 24, 2023

BDisp commented Dec 24, 2023

tig commented Dec 24, 2023

BDisp commented Dec 24, 2023 • edited Loading

tznind commented Dec 24, 2023

dodexahedron commented Dec 24, 2023

dodexahedron commented Dec 24, 2023

BDisp commented Dec 24, 2023

This comment was marked as off-topic.

BDisp commented Dec 24, 2023

This comment was marked as off-topic.

tig commented Dec 24, 2023 • edited Loading

tznind commented Dec 24, 2023 via email

BDisp commented Dec 24, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

BDisp commented Dec 25, 2023

dodexahedron commented Dec 25, 2023

dodexahedron commented Dec 25, 2023 • edited Loading

Footnotes

tig commented Dec 25, 2023

Casting inconsistent between `Key` and `KeyCode` #3071

Casting inconsistent between `Key` and `KeyCode` #3071

BDisp commented Dec 23, 2023 •

edited

Loading

BDisp commented Dec 23, 2023 •

edited

Loading

tig commented Dec 23, 2023 •

edited

Loading

dodexahedron commented Dec 24, 2023 •

edited

Loading

BDisp commented Dec 24, 2023 •

edited

Loading

dodexahedron commented Dec 24, 2023 •

edited

Loading

dodexahedron commented Dec 24, 2023 •

edited

Loading

tig commented Dec 24, 2023 •

edited

Loading

BDisp commented Dec 24, 2023 •

edited

Loading

tig commented Dec 24, 2023 •

edited

Loading

dodexahedron commented Dec 25, 2023 •

edited

Loading