Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 77 commits into from
Oct 31, 2022

Conversation

tznind
Copy link
Collaborator

@tznind tznind commented Sep 17, 2022

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

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
  • [ x I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

…onsoleKey.Packet. Affects only WindowsDriver
@tznind
Copy link
Collaborator Author

tznind commented Sep 17, 2022

@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

@heinrich-ulbricht
Copy link

Thank you, will do this weekend!

@heinrich-ulbricht
Copy link

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

@BDisp
Copy link
Collaborator

BDisp commented Sep 18, 2022

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

@heinrich-ulbricht
Copy link

heinrich-ulbricht commented Sep 18, 2022

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

@BDisp
Copy link
Collaborator

BDisp commented Sep 18, 2022

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.

@heinrich-ulbricht
Copy link

heinrich-ulbricht commented Sep 18, 2022

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

@tznind
Copy link
Collaborator Author

tznind commented Sep 18, 2022

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

@heinrich-ulbricht
Copy link

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

@tznind
Copy link
Collaborator Author

tznind commented Sep 18, 2022

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

@heinrich-ulbricht
Copy link

heinrich-ulbricht commented Sep 18, 2022

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

@tznind
Copy link
Collaborator Author

tznind commented Sep 19, 2022

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

A = 65,
/// <summary>
/// The key code for the user pressing Shift-B
/// </summary>
B,
/// <summary>
/// The key code for the user pressing Shift-C
/// </summary>
C,

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

/// <summary>
/// The key value cast to an integer, you will typical use this for
/// extracting the Unicode rune value out of a key, when none of the
/// symbolic options are in use.
/// </summary>
public int KeyValue => (int)Key;

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
Copy link
Collaborator Author

tznind commented Sep 19, 2022

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 #2008 (comment) . 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.

@heinrich-ulbricht
Copy link

heinrich-ulbricht commented Sep 19, 2022

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

@BDisp
Copy link
Collaborator

BDisp commented Sep 19, 2022

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.

@heinrich-ulbricht
Copy link

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

@BDisp
Copy link
Collaborator

BDisp commented Sep 19, 2022

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

@tznind tznind marked this pull request as ready for review September 20, 2022 06:05
@tznind
Copy link
Collaborator Author

tznind commented Sep 20, 2022

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);

@tig
Copy link
Collaborator

tig commented Sep 20, 2022

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

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

@tznind I submitted the PR tznind#128 to your branch. I already merged the develop branch because of the resulting conflicts. All drivers, except the CursesDriver are prepared to receive Packet keys. The CursesDriver only simulate through the SendKeys method. The reason is the keys are only caught in the ProcessInput method with the Curses.get_wch method, while the others drivers the keys are passed on the parameter. I really don't know if ncurses can handle the virtual packet keys.
I also created a VkeyPacketSimulator scenario which supports all the drivers. Typing in the firstTextView will write in the second TextView.

@tznind
Copy link
Collaborator Author

tznind commented Oct 30, 2022

This is awesome work thanks! I have merged the PR and will get to testing it / looking in depth asap.

Do you consider this now as good to go? shall I mark it no longer Draft?

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

This is awesome work thanks! I have merged the PR and will get to testing it / looking in depth asap.

Thanks.

Do you consider this now as good to go? shall I mark it no longer Draft?

I forgot to delete the CanShiftBeAdded from the FakeDriver. I'll submit a new PR to fix it. After you merge you can mark as ready, please.

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

Done tznind#134. Thanks for your waiting.

Removed unnecessary method.
@tznind tznind marked this pull request as ready for review October 30, 2022 21:41
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shift+key denied by text field as "control character" on remote systems
5 participants