Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

Fixes #2008 - Shift+key denied by text field as "control character" on remote systems

Open tznind opened this issue 1 year ago • 39 comments

Fixes #2008 - When using some remoting technologies some keyboard input is supplied as ConsoleKey.Packet.

Pull Request checklist:

  • [x] 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.
  • [x] My code follows the Terminal.Gui library design guidelines
  • [x] I ran dotnet test before commit
  • [x] I have made corresponding changes to the API documentation (using /// style comments)
  • [x] My changes generate no new warnings
  • [ x I have checked my code and corrected any poor grammar or misspellings
  • [ ] I conducted basic QA to assure all features are working

tznind avatar Sep 17 '22 12:09 tznind

@heinrich-ulbricht please are you able to test my PR to see if this has any effect on your Shift+char issue with TextView / TextField etc.

I have gone to the point where the keystrokes first appear in WindowsDriver and made a conservative edit that tries to remap the Packet virtual key so the virtual input keystrokes should now look identical to regular keystrokes from then on.

Since I don't have a machine that sends these PACKET entries I can't test it myself. But I have written a couple of unit tests and if successful I can write some more to make sure nothing goes wrong when trying to remap wierd characters e.g. 'newline' or 'escape' etc.

I've left it as draft for now since I want to see that

  • It definitely fixes your problem
  • There are no corner cases where the remapping will map to an invalid/different Enum.

For the remapping I followed this thread on StackOverflow. But adjusted it to correctly handle lower case too.

Test is written to use InlineData so its easy to expand for the strange symbols etc

tznind avatar Sep 17 '22 12:09 tznind

Thank you, will do this weekend!

heinrich-ulbricht avatar Sep 17 '22 15:09 heinrich-ulbricht

@tznind Looks promising. It works for A, B etc. But not for : and other non-alphabet characters. For those the Enum.TryParse fails.

heinrich-ulbricht avatar Sep 18 '22 12:09 heinrich-ulbricht

@tznind Looks promising. It works for A, B etc. But not for : and other non-alphabet characters. For those the Enum.TryParse fails.

May be for them is not necessary to be mapped and come with the correct key code? So only try parse from A to Z?

BDisp avatar Sep 18 '22 12:09 BDisp

Had a look at NetDriver and what the ConsoleKeyInfo there looks like after pressing Shift+'.' (== ':'). The KeyChar there is set, Key is 0, no modifier:

-		consoleKeyInfo	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	0	System.ConsoleKey
		KeyChar	58 ':'	char
		Modifiers	0	System.ConsoleModifiers

But the same is true for 'A'.

heinrich-ulbricht avatar Sep 18 '22 12:09 heinrich-ulbricht

The NetDriver works in a different way. It read all the keys come from a virtual terminal and also read characters sequences. The Shift key is irrelevant from A to Z, because we want to receive the letters from A to Z or from a to z and we don't need to check if it has the shift modifier or not. Also for others keys that are return by pressing the shift+char the keyboard driver also send the right key. We only need to check for the shift key if it's used with more keys, like Ctrl and Alt, or it's used with another char that isn't modified with the shift key, like Shift+CursorLeft and so on. But this keys come encoded with the PACKET key and need to be decode, but perhaps for the keys from A to Z and not for others that depend of the keyboard language specific.

BDisp avatar Sep 18 '22 13:09 BDisp

Some tests:

Ctrl

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	17	System.ConsoleKey
		KeyChar	0 '\0'	char
		Modifiers	Control	System.ConsoleModifiers

Ctrl+Shift

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	16	System.ConsoleKey
		KeyChar	0 '\0'	char
		Modifiers	Shift | Control	System.ConsoleModifiers

Ctrl+Shift+A

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	A	System.ConsoleKey
		KeyChar	1 '\u0001'	char
		Modifiers	Shift | Control	System.ConsoleModifiers

Shift+7 == '/'

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	47 '/'	char
		Modifiers	Shift	System.ConsoleModifiers

According to both ConsoleKey and Key enums 47 is the "help key"... Ahm nope. ASCII 47 is '/' indeed. So this speaks against converting the KeyChar to a ConsoleKey.

Shift+a == 'A'

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	65 'A'	char
		Modifiers	Shift	System.ConsoleModifiers

Shift+ß == '?'

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	63 '?'	char
		Modifiers	Shift	System.ConsoleModifiers

A German speciality: ß

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	223 'ß'	char
		Modifiers	0	System.ConsoleModifiers

A classic: ü

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	252 'ü'	char
		Modifiers	0	System.ConsoleModifiers

u

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	117 'u'	char
		Modifiers	0	System.ConsoleModifiers

Return/Enter

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Enter	System.ConsoleKey
		KeyChar	13 '\r'	char
		Modifiers	0	System.ConsoleModifiers

Space

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Spacebar	System.ConsoleKey
		KeyChar	32 ' '	char
		Modifiers	0	System.ConsoleModifiers

Right arrow

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	RightArrow	System.ConsoleKey
		KeyChar	0 '\0'	char
		Modifiers	0	System.ConsoleModifiers

Home

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Home	System.ConsoleKey
		KeyChar	0 '\0'	char
		Modifiers	0	System.ConsoleModifiers

Ctrl+Pos1

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Home	System.ConsoleKey
		KeyChar	0 '\0'	char
		Modifiers	Control	System.ConsoleModifiers

Ctrl+q

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Q	System.ConsoleKey
		KeyChar	17 '\u0011'	char
		Modifiers	Control	System.ConsoleModifiers

heinrich-ulbricht avatar Sep 18 '22 14:09 heinrich-ulbricht

@tznind Looks promising. It works for A, B etc. But not for : and other non-alphabet characters. For those the Enum.TryParse fails.

That's great @heinrich-ulbricht thanks for testing. I suspected that TryParse was a bit hacky. I'll just write a switch statement for the whole ConsoleKey Enum (at least where theres a compatible letter key and skip A-Z since they work with TryParse). There are only ~100 entries so shouldn't be to onerous.

I suspect for the foreign language keys there might still be issues. But since you can now type the US letters, this approach seems sensible to start with.

Your comment is very helpful where it shows what is and isn't working, I can add those to the unit test I created.

tznind avatar Sep 18 '22 15:09 tznind

@tznind I have URL input fields so as long as one can type URLs it's fine for me :zany_face:

heinrich-ulbricht avatar Sep 18 '22 17:09 heinrich-ulbricht

@heinrich-ulbricht please can you see how it is behaving now.

I have mapped every key on my US layout keyboard and added the novel characters that appear in UK keyboard (since I also have one of those to test with).

I'm not sure what to do with characters that appear in different keys on different keyboard layouts (e.g. " and @ are on different keys depending on layout).

I didn't seem to see all the Oem keys so there might be some additional mappings that could be added. And there are clearly some keys that produce the same symbol e.g. . which appears on both the > key and the numerical keypad. I think we get a flag for that though so maybe that could be looked into (or it may not really matter which is used).

It seems that some of the code I am writting is similar to public Key MapKey (WindowsConsole.ConsoleKeyInfoEx keyInfoEx) so I will probably also dig deeper to make sure I'm not rewritting duplicate code.

tznind avatar Sep 18 '22 18:09 tznind

@tznind It works better now, I can input URLs ^^ Pressed a few more keys, single and double quotes are missing:

-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	34 '"'	char
		Modifiers	Shift	System.ConsoleModifiers
-		original	{System.ConsoleKeyInfo}	System.ConsoleKeyInfo
		Key	Packet	System.ConsoleKey
		KeyChar	39 '\''	char
		Modifiers	Shift	System.ConsoleModifiers

Somehow this approach doesn't feel right, though. I read that the packet key type was introduced to not have to deal with different keyboard layouts when sending remote key presses around. To "just pass the actual key through" without going through multiple transformations. It would be nice if we could do that. But this again somehow feels like it might require a bigger architectural change. Edit: Thought about that. If the final console output IS dependent on those enums then there is no choice.

As an immediate workaround it definitely solves the (my) problem.

heinrich-ulbricht avatar Sep 18 '22 18:09 heinrich-ulbricht

Somehow this approach doesn't feel right, though. I read that the packet key type was introduced to not have to deal with different keyboard layouts when sending remote key presses around. heinrich-ulbricht

I agree this definetly feels a bit sketchy. I'm very much open to talking more about this though!

Terminal.Gui keyboard events

All the View in Terminal.Gui deal with KeyEvent which is a wrapper for Key and KeyModifiers

The Key Enum

Key is an Enum declared and defined in Terminal.Gui

The Key enumeration contains special encoding for some keys, but can also encode all the unicode values that can be passed.

This means that any char can be stored in references to Key even if they don't have an explicit declared entry. There is a ticket already open in discussion to add many of the missing values.

Because Key is using the Unicode/ASCII numbers for entries it doesn't care about keyboard layout

https://github.com/gui-cs/Terminal.Gui/blob/558b2b793f700dec5672536b934300b86148bfe7/Terminal.Gui/Core/Event.cs#L139-L147

The KeyEvent class is explicitly set up with the expectation that unmapped Key values are used e.g.:

https://github.com/gui-cs/Terminal.Gui/blob/558b2b793f700dec5672536b934300b86148bfe7/Terminal.Gui/Core/Event.cs#L487-L492

ConsoleKey

ConsoleKeyInfo only appears in the implementations of ConsoleDriver (i.e. NetDriver and WindowsDriver) and not in the rest of the Terminal.Gui codebase which is good!

Summary

So the goal here is to correctly convert ConsoleKeyInfo.Packet + char (unicode) into the correct Key enum entry. This involves 2 things

  • If the char is \b or \t or \u001b (escape) or \r then we should turn it into the correct Key entry (unless those already match on uint value).
  • If the char is any printable character we should just cast it to Key and not worry that it doesn't have an explicit entry

I think my going via ConsoleKeyInfo was a mistake since it seems tied to keyboard layouts and probably over complicating things. Especially since 99% of keystrokes should just be castable to Key as uint.

tznind avatar Sep 19 '22 09:09 tznind

Ok @heinrich-ulbricht please can you try updating now! I have reworked the code to map to Key instead of ConsoleKey which turns out is way simpler than what I had done before.

I think your keyboard will not use Packet for arrow keys and Home/End etc. Assuming regular typing is working ok, can you please test Home/End/Cursor keys in the debugger like you did here https://github.com/gui-cs/Terminal.Gui/issues/2008#issuecomment-1247071927 . Since there isn't a unicode symbol for Home (I think) I don't think it will use Packet (it wouldn't make much sense).

I don't know if it even uses Packet for Escape / Tab and Backspace but I've put those cases in anyway in TryRemapPacketKey because the unicode value and the entry in Key Enum are not the same (I think).

Thanks for helping test this, it feels like we are nearly there.

tznind avatar Sep 19 '22 10:09 tznind

@tznind Well. This just works :D

I can type anything in combination with Shift now. No exceptions. Home, End, Cursors, Escape, Tab, Backspace also work and don't use the Packet type.

Here are some key events taken from ToConsoleKeyInfoEx, just say if you need more:

Home

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	36	ushort
		wVirtualScanCode	71	ushort

End

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	35	ushort
		wVirtualScanCode	79	ushort

Right arrow

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	39	ushort
		wVirtualScanCode	77	ushort

ESC

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	27 '\u001b'	char
		bKeyDown	true	bool
		dwControlKeyState	0	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	27	ushort
		wVirtualScanCode	1	ushort

Shift

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	ShiftPressed	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	16	ushort
		wVirtualScanCode	42	ushort

Shift + a pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	65 'A'	char
		bKeyDown	true	bool
		dwControlKeyState	ShiftPressed	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	231	ushort
		wVirtualScanCode	0	ushort

Shift + Tab pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	9 '\t'	char
		bKeyDown	true	bool
		dwControlKeyState	ShiftPressed	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	9	ushort
		wVirtualScanCode	15	ushort

Return

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	13 '\r'	char
		bKeyDown	true	bool
		dwControlKeyState	0	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	13	ushort
		wVirtualScanCode	28	ushort

Ctrl + Home pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	LeftControlPressed | EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	36	ushort
		wVirtualScanCode	71	ushort

Shift + Home pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	ShiftPressed | EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	36	ushort
		wVirtualScanCode	71	ushort

Shift + End pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	0 '\0'	char
		bKeyDown	true	bool
		dwControlKeyState	ShiftPressed | EnhancedKey	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	35	ushort
		wVirtualScanCode	79	ushort

Ctrl + a pressed

-		keyEvent	{Terminal.Gui.WindowsConsole.KeyEventRecord}	Terminal.Gui.WindowsConsole.KeyEventRecord
		UnicodeChar	1 '\u0001'	char
		bKeyDown	true	bool
		dwControlKeyState	LeftControlPressed	Terminal.Gui.WindowsConsole.ControlKeyState
		wRepeatCount	1	ushort
		wVirtualKeyCode	65	ushort
		wVirtualScanCode	30	ushort

By the way - is Ctrl+A (to select all text) supposed to work in input fields? Because it doesn't. Just something I noticed a while back, not sure if this is connected to our current issue. Just want to mention it just in case. Shift+Home and Shift+End do work. Edit: My bad, just saw in the context menu that Ctrl+T is "Select all".

Thanks for diving into this. This solution feels a lot better.

heinrich-ulbricht avatar Sep 19 '22 20:09 heinrich-ulbricht

By the way - is Ctrl+A (to select all text) supposed to work in input fields? Because it doesn't. Just something I noticed a while back, not sure if this is connected to our current issue. Just want to mention it just in case. Shift+Home and Shift+End do work. (Note: Is this a unicode character there for Ctrl+a that needs special handling?)

On TextField and TextView to select all text is by pressing Ctrl+T.

BDisp avatar Sep 19 '22 20:09 BDisp

@BDisp You are fast. Just this moment I pressed the right mouse key and the context menu told me this :) Strange combo, my Windows background strongly expects Ctrl+A ^^

heinrich-ulbricht avatar Sep 19 '22 20:09 heinrich-ulbricht

Ctrl+A is for Command.LeftHome on TextField and for Command.StartOfLine on TextView.

BDisp avatar Sep 19 '22 20:09 BDisp

So it looks like the packet key is not used in any of these cases (below) for you. So the following code I have in this PR will probably never be hit so could be removed in your case. But I don't know if other software or environments might use PACKET for any of these. @tig let me know if you want me to remove this code or leave it in (or anyone else who has a view!).

case '\t':
	result = original.Modifiers == ConsoleModifiers.Shift ? Key.BackTab : Key.Tab;
	return true;
case '\u001b':
	result = Key.Esc;
	return true;
case '\b':
	result = Key.Backspace;
	return true;
case '\n':
case '\r':
	result = Key.Enter;
	return true;

@heinrich-ulbricht one last thing I thought I'd mention incase you hadn't seen - You can change the keybinding for SelectAll with:

TextField tv = new TextField ();
tv.AddKeyBinding (Key.A | Key.CtrlMask, Command.SelectAll);

tznind avatar Sep 20 '22 06:09 tznind

@tig let me know if you want me to remove this code or leave it in (or anyone else who has a view!).

Nuke it. ;-)

tig avatar Sep 20 '22 14:09 tig

Ok I have nuked the switch. @heinrich-ulbricht if you have the time please to do a final check that I haven't missed something (i.e. that your configuration still works correctly for all typing).

tznind avatar Sep 20 '22 15:09 tznind

Somehow this approach doesn't feel right, though. I read that the packet key type was introduced to not have to deal with different keyboard layouts when sending remote key presses around. To "just pass the actual key through" without going through multiple transformations. It would be nice if we could do that. But this again somehow feels like it might require a bigger architectural change. Edit: Thought about that. If the final console output IS dependent on those enums then there is no choice.

As an immediate workaround it definitely solves the (my) problem.

This justification intrigued me and reflected if they created this Packet Key was precisely so there was no conflict with any keyboard layout. As this situation only happened to WindowsDriver so I thought the problem was just like this driver handles Shift Key.

So I thinking commenting this two lines on the WindowsDriver fix that problem because the Shift key doesn't need to go into the Key at this moment but only int the modifiers.

https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/WindowsDriver.cs#L1377-L1378

I also commented on these two on the FakeDriver and all the current unit tests passed.

https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/FakeDriver/FakeDriver.cs#L363-L364

@heinrich-ulbricht since only you are using that remote tool can you do this changes to the develop branch and test if the Shift key is processed well in all situations, please? Thanks.

BDisp avatar Sep 20 '22 20:09 BDisp

I changed the unit test like bellow and it passes if the above lines are commented, otherwise fails. We need to replace the shift parameter to true.

[Theory, AutoInitShutdown]
[InlineData ('A', Key.A)]
[InlineData ('z', Key.z)]
[InlineData ('=', (Key)'=')]
[InlineData ('英', (Key)'英')]
[InlineData ('+', (Key)'+')]
[InlineData ('\0', (Key)'\0')]
public void TestVKPacket (char unicodeCharacter, Key expectedRemapping)
{
	var before = new ConsoleKeyInfo (unicodeCharacter, ConsoleKey.Packet, true, false, false);
	var top = Application.Top;

	top.KeyPress += (e) => {
		var after = e.KeyEvent.Key;
		Assert.Equal (before.KeyChar, (char)after);
		Assert.Equal (expectedRemapping, after);
	};

	Application.Begin (top);

	Application.Driver.SendKeys (unicodeCharacter, ConsoleKey.Packet, true, false, false);
}

BDisp avatar Sep 20 '22 21:09 BDisp

I'm pretty sure the culprit of the issue was as demonstrated in the previous comment.

BDisp avatar Sep 20 '22 21:09 BDisp

So you mean changing the Shift handling could be alternative solution?

And for the record my test setting:

  • Fedora + Remmina for RDP
  • any Windows VM on Azure to RDP to
  • UICatalog from the current repo running on the Windows machine

That's it, not sooo esoteric. But yes, I seem to be the only one doing this with Terminal.Gui :D

I'll check.

heinrich-ulbricht avatar Sep 20 '22 22:09 heinrich-ulbricht

So you mean changing the Shift handling could be alternative solution?

Yes. In Windows Froms normally we check for a KeyChar with the keys modifiers, but Terminal.Gui is very tied with the shifted keys, like Key.A | Key.CtrlMask, instead of KeyChar == 'A' && Modifiers.Control. I tried to change that behavior before but I'm started having many conflicts and errors and I gave up.

And for the record my test setting:

* Fedora + Remmina for RDP

* any Windows VM on Azure to RDP to

* UICatalog from the current repo running on the Windows machine

That's it, not sooo esoteric. But yes, I seem to be the only one doing this with Terminal.Gui :D

Yes i think you are the only one doing that :-) I don't have Azure and have no idea how to setup something like that.

I'll check.

Thanks.

BDisp avatar Sep 20 '22 23:09 BDisp

Instead of commenting the lines related above is better do the follow:

Changing this: https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/WindowsDriver.cs#L1367-L1369

To this:

	if (keyInfo.KeyChar != 0) {
		if (keyInfo.Key == ConsoleKey.Packet) {
			return (Key)((uint)keyInfo.KeyChar);
		} else {
			return MapKeyModifiers (keyInfo, (Key)((uint)keyInfo.KeyChar));
		}
	}

and changing this: https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/FakeDriver/FakeDriver.cs#L351-L353

to this:

	if (keyInfo.KeyChar != 0) {
		if (keyInfo.Key == ConsoleKey.Packet) {
			return (Key)((uint)keyInfo.KeyChar);
		} else {
			return MapKeyModifiers (keyInfo, (Key)((uint)keyInfo.KeyChar));
		}
	}

BDisp avatar Sep 21 '22 02:09 BDisp

Another better solution and as this issue only affect the Packet key used with the Shift key it's more accurate only not shifted the Shift key with the ConsoleKeyInfo.KeyChar if the ConsoleKeyInfo.Key is the ConsoleKey.Packet.

So, in this lines: https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/WindowsDriver.cs#L1377-L1378

Change to this:

	if (keyInfo.Key != ConsoleKey.Packet && (keyInfo.Modifiers & ConsoleModifiers.Shift) != 0)
		keyMod = Key.ShiftMask;

In this lines: https://github.com/gui-cs/Terminal.Gui/blob/deef59e877b7889c59b317eb0dccc03465e0f8ad/Terminal.Gui/ConsoleDrivers/FakeDriver/FakeDriver.cs#L363-L364

Change to this:

	if (keyInfo.Key != ConsoleKey.Packet && (keyInfo.Modifiers & ConsoleModifiers.Shift) != 0)
		keyMod = Key.ShiftMask;

BDisp avatar Sep 21 '22 10:09 BDisp

Sounds like you have a simpler solution than this PR @BDisp. It looks good but I'm sort of loosing track of all the edits your suggesting.

Do you want to create a new PR with your changes? I'm happy to go with adjusting the existing logic instead of my additional logic if that is more correct.

tznind avatar Sep 21 '22 10:09 tznind

But we have to be sure if my solution is more accurate. Please, change your unit test to this and confirm if it must pass.

[Theory]
[InlineData ('A', false, false, false, Key.A)]
[InlineData ('A', true, false, false, Key.A)]
[InlineData ('A', true, true, false, Key.A | Key.AltMask)]
[InlineData ('A', true, true, true, Key.A | Key.AltMask | Key.CtrlMask)]
[InlineData ('z', false, false, false, Key.z)]
[InlineData ('z', true, false, false, Key.z)]
[InlineData ('z', true, true, false, Key.z | Key.AltMask)]
[InlineData ('z', true, true, true, Key.z | Key.AltMask | Key.CtrlMask)]
[InlineData ('=', false, false, false, (Key)'=')]
[InlineData ('=', true, false, false, (Key)'=')]
[InlineData ('=', true, true, false, (Key)'=' | Key.AltMask)]
[InlineData ('=', true, true, true, (Key)'=' | Key.AltMask | Key.CtrlMask)]
[InlineData ('英', false, false, false, (Key)'英')]
[InlineData ('英', true, false, false, (Key)'英')]
[InlineData ('英', true, true, false, (Key)'英' | Key.AltMask)]
[InlineData ('英', true, true, true, (Key)'英' | Key.AltMask | Key.CtrlMask)]
[InlineData ('+', false, false, false, (Key)'+')]
[InlineData ('+', true, false, false, (Key)'+')]
[InlineData ('+', true, true, false, (Key)'+' | Key.AltMask)]
[InlineData ('+', true, true, true, (Key)'+' | Key.AltMask | Key.CtrlMask)]
[InlineData ('\0', false, false, false, (Key)'\0')]
[InlineData ('\0', true, false, false, (Key)'\0')]
[InlineData ('\0', true, true, false, (Key)'\0' | Key.AltMask)]
[InlineData ('\0', true, true, true, (Key)'\0' | Key.AltMask | Key.CtrlMask)]
public void TestVKPacket (char unicodeCharacter, bool shift, bool alt, bool control, Key expectedRemapping)
{
	var before = new ConsoleKeyInfo (unicodeCharacter, ConsoleKey.Packet, shift, alt, control);
	Assert.True (WindowsDriver.TryRemapPacketKey (before, out var after));

	// The thing we are really interested in, did we correctly convert
	// the input ConsoleKey.Packet to the correct physical key
	Assert.Equal (expectedRemapping, after);
}

BDisp avatar Sep 21 '22 11:09 BDisp

The WindowsDriver is the only driver that really processes the key down and the key up events, by check the bKeyDown field. The others drivers processes that by simulation. Only on WindowsDriver is possible to get the modifiers keys directly by the library, the others drivers are dependent of the result from the Terminal.Gui.Key. So the modifiers keys in the WindowsDriver are very reliable.

BDisp avatar Sep 21 '22 12:09 BDisp