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

tig · 2023-12-25T15:05:48Z

Adds new Key (char) constructor
Updates (char)Key cast to use char constructor
Adds unit tests
static standard key properties now always get new instance
Adds Key constructor that takes a string
Changes KeyJsonSerializer to be far simpler (no object) and public. Removes it as an attribute on Key

Pull Request checklist:

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

BDisp · 2023-12-25T23:26:22Z

Terminal.Gui/Input/Key.cs

+	public Key (char ch)
+	{
+		switch (ch) {
+		case >= 'A' and <= 'Z':
+			// Upper case A..Z mean "Shift-char" so we need to add Shift
+			KeyCode = (KeyCode)ch | KeyCode.ShiftMask;
+			break;
+		case >= 'a' and <= 'z':
+			// Lower case a..z mean no shift, so we need to store as Key.A...Key.Z
+			KeyCode = (KeyCode)(ch - 32);
+			return;
+		default:
+			KeyCode = (KeyCode)ch;
+			break;
+		}
+	}


@tig you can leverage this code https://github.com/BDisp/Terminal.Gui/blob/9349d542974c6eb59044b3ca4716cc1a25101321/Terminal.Gui/Input/Key.cs#L195-L253 to get the shift or not based on more range of characters to allow test this:

[InlineData ('á', (KeyCode)'á')] [InlineData ('Á', (KeyCode)'Á' | KeyCode.ShiftMask)]

Here is the full code https://github.com/BDisp/Terminal.Gui/blob/9349d542974c6eb59044b3ca4716cc1a25101321/Terminal.Gui/ConsoleDrivers/ConsoleKeyMapping.cs#L196-L294

I don't think that is correct behavior. Is it really needed for VK packet support or is it something you think is a nice-to-have?

If it's not absolutely needed for VK packet support then I'd prefer to have A..Z be the only keys supported in the test.

Ah if it's only needed for the tests ok, but you still need this for TextView recognize that they are valid characters to pass on the GetIsKeyCodeAtoZ method to insert the text.

An aside, as I've been looking at all these tests:

Might I suggest using MemberData to generate test cases instead of all the InlineData?

It's xUnit's analogue to NUnit's TestCaseSource and would allow you to have more exhaustive and authoritative tests with less work and less code.

The general pattern is:

[Theory] [MemberData(nameof(TheoryA_Cases))] public void TheoryA( string s, int i, object o ) { // Test code } private static IEnumerable<object[]> TheoryA_Cases() { // Code that provides an IEnumerable of 3-element object arrays that satisfy the parameters of the test. }

xUnit's documentation is sorely lacking, but that's the basic pattern for using that.

I do not know if xUnit has a way to generate tests from combinatorial parameter sources, but that effect can be achieved by just doing the combinatorial loops yourself in the case generator method.

Right and that's fine, because the information is still there. As long as you can round-trip it, you're golden.

The only exceptions where a shift key can be added alone to a Key is when a non-printable character is used, like F1..F12, cursor keys, delete, backspace, etc.

Makes perfect sense.

Theoretical question, though:

Say I have a string on the clipboard that has all characters my keyboard can produce - letters, digits, symbols, and whitespace - and I paste that string while holding ctrl.

Would characters such as the ! be interpreted as "ctrl+shift+D1"? Or would they come across as just D1? Or would they come across as ctrl+D1? Or do they not even take that path and only ever come in as a literal "!"?🤔

In my PR I was creating as (KeyCode)'!' | KeyCode.ShiftMask to leverage the character. ConsoleKeyInfo parses it as KeyChar='!', Key=ConsoleKey.D1, ConsoleModifiers.Shift.
Pasting from the clipboard only will return the clipboard content and not the ctrl key and will return as (KeyCode)'!' | KeyCode.ShiftMask. In TextField you use the Ctrl+V to paste and in TextView you use the Ctrl+Y.
But now the problem adding only a shift key on a character other than A..Z, because the shift key is removed from the Key before insert the text in a TextView and not for the others characters. Because of this issue, Terminal.Gui never added before the shift key alone on printable character. Now it's only possible to A..Z and thus is impossible normalize all.

Tell me the user scenario that no longer works.

I don't think that is correct behavior. Is it really needed for VK packet support or is it something you think is a nice-to-have?

With the current Key code is the correct behavior if the GetIsKeyCodeAtoZ is to persist always because the shift key does not is present only from A..Z. It's good to have, although the shift key must be removed before sending to a consumer and the IsShift indicates whether the shift key was pressed.

If it's not absolutely needed for VK packet support then I'd prefer to have A..Z be the only keys supported in the test.

With the current Key code that only is supporting the shift key for A..Z, this is needed for the VK packet support as well.
I already proved that 'Ó' is (Key)'Ó' | Key.Shift but there are also others printable character that has the shift key, like !"#$%&/()=?»*ª>;:_. There are two options:

Not including only the shift key to any printable character as before which will not provide the IsShift property as true.

Including the shift key to any printable character which will provide the IsShift property as true. I prefer this one but if the first is preferable the the GetIsKeyCodeAtoZ must be removed and treat all as equal as before.

dodexahedron · 2023-12-26T00:37:54Z

What do you think of maybe also adding a constructor taking string that essentially operates as a Parse method would, and/or adding a Parse method (ie a normal/non-try version of TryParse)? I'm always a fan of implementing the TryX pattern, but Try and non-Try versions each have their valid use cases, and providing both requires no code duplication, since one can simply delegate to the other.

"Regular" Parse methods also provide the opportunity to communicate to the caller why the parse failed, rather than simply that it failed, if appropriate exceptions are thrown, whereas a TryParse method requires stepping through foreign source to trace the cause of a false result.

tig · 2023-12-26T00:58:14Z

💯 👆

dodexahedron · 2023-12-26T03:10:22Z

I'm also finding, at least from the usage in TerminalGuiDesigner, as I'm adapting it to match the new Key type, that an implicit cast operator to and from string that use ToString() and Parse() respectively would be a pretty nice thing to have, and would feel really natural in use. It would also make configuration a ton easier, since M.E.C. looks for string conversions for incoming data to the declared type.

tig · 2023-12-26T15:37:33Z

I'm also finding, at least from the usage in TerminalGuiDesigner, as I'm adapting it to match the new Key type, that an implicit cast operator to and from string that use ToString() and Parse() respectively would be a pretty nice thing to have, and would feel really natural in use. It would also make configuration a ton easier, since M.E.C. looks for string conversions for incoming data to the declared type.

Done.

BDisp · 2023-12-26T22:37:32Z

UnitTests/Input/KeyTests.cs

+	[InlineData ("Shift+A", KeyCode.A | KeyCode.ShiftMask)]
+	[InlineData ("A", KeyCode.A | KeyCode.ShiftMask)]
+	[InlineData ("â", (KeyCode)'â')]
+	[InlineData ("Shift+â", (KeyCode)'â' | KeyCode.ShiftMask)]


This is wrong. 'â' is the lower case of 'Â'. So it haven't the shift key. Only the 'Â' has the shift key. Because of only consider the A..Z for the test the logic of the shift key is broken.

tig · 2023-12-27T22:50:33Z

@BDisp I'm going to merge this now, ahead of me reviewing the VKPacket PR. I can't keep my changes and your suggestions straight in my head as-is.

I know we disagree on something fundamental, and I'm eager to understand the crux of our disagreement, but I don't have clarity yet.

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

tig added 2 commits December 25, 2023 06:51

Removed char->Key cast. Added Key(char)

9aa256b

Re-added char->key cast. Added unit tests

4941afc

tig mentioned this pull request Dec 25, 2023

Casting inconsistent between Key and KeyCode #3071

Closed

Fixed standard keys to always return new instead of being readonly

7897ea1

tig changed the title ~~Fixes #3071. Key cast is not correct.~~ Fixes #3079. Key cast is not correct. Dec 25, 2023

tig changed the title ~~Fixes #3079. Key cast is not correct.~~ Fixes #3071 & #3079. Key cast is not correct. Dec 25, 2023

tig changed the title ~~Fixes #3071 & #3079. Key cast is not correct.~~ Fixes #3071 & #3079. Key cast and static props are not correct Dec 25, 2023

BDisp reviewed Dec 25, 2023

View reviewed changes

dodexahedron mentioned this pull request Dec 26, 2023

Convert unit tests to Constraint model dodexahedron/TerminalGuiDesigner#1

Open

Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up

315353d

tig added 3 commits December 26, 2023 07:20

Re-fixed WindowsDriver to report shift/alt/ctrl as key/down/up

01dfa63

Adds string constructor to Key + tests.

d7ef966

Simplified Key json

546d948

tig mentioned this pull request Dec 26, 2023

Enable overriding default Key Bindings with ConfigurationManager #3023

Open

Added string/Key cast operators.

e67ecb4

BDisp suggested changes Dec 26, 2023

View reviewed changes

tig merged commit 80ef4b5 into gui-cs:v2_develop Dec 27, 2023

Copilot AI mentioned this pull request Oct 3, 2025

Design: Enable overriding default Key Bindings with ConfigurationManager #4266

Draft

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

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

Uh oh!

Conversation

tig commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist:

Uh oh!

BDisp Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodexahedron Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BDisp Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodexahedron commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tig commented Dec 26, 2023

Uh oh!

dodexahedron commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tig commented Dec 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tig commented Dec 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tig commented Dec 25, 2023 •

edited

Loading

BDisp Dec 25, 2023 •

edited

Loading

dodexahedron Dec 26, 2023 •

edited

Loading

BDisp Dec 26, 2023 •

edited

Loading

dodexahedron commented Dec 26, 2023 •

edited

Loading

dodexahedron commented Dec 26, 2023 •

edited

Loading