Fixes key handling for Windows. #172

tig · 2024-01-02T00:38:55Z

@BDisp this is still a work in progress.

I broke ENTER key handling in Textview but this is now on the right track, i think.

I need to take a break, so I'm submitting this before it's really ready.

I did not touch the VKeySimulator because A ton of the underlying code is simply wrong and needs to be redone.

Note that the whole VK_Packet stuff is only relevant on Windows (and perhaps only WindowsDriver). the Scenario should NOT even attempt to work on Linux IMO.

* Add ToString implementation for SliderOption<T> * Add test case for SliderOption ToString with more complex class

* Add constructor overload to SliderOption * Adjust whitespace * Move tests to SliderOptionTests * Empty commit for CI

…rminal.Gui into BDisp-v2_vkeypacketsimulator-fix_3054

… codepoint

BDisp · 2024-01-02T00:44:38Z

Note that the whole VK_Packet stuff is only relevant on Windows (and perhaps only WindowsDriver). the Scenario should NOT even attempt to work on Linux IMO.

I disagree because it also work on Linux. Only some key bindings with shift aren´t recognized because of the SO itself but the all the rest work.
I'll take a look tomorrow, time to sleep. Take a break too. Thanks.

tig · 2024-01-02T01:15:14Z

VK_PACKET is literally a virtual key defined in winuser.h.

There is no way for such a thing to be surfaced in curses.

However if you mean the simulator can work on Linux that's a different point... that I fail to see the point of. I also don't see what api you'd use to map KeyCodes to codes chars without a Linux equivalent of MapVirtualKeyCode.

Or, if you mean netdriver in Linux: how do we get the keyboard layout there?

BDisp · 2024-01-02T05:14:29Z

It's defined in dotnet as well in the ConsoleKey.Packet.

tig · 2024-01-02T15:52:29Z

It's defined in dotnet as well in the ConsoleKey.Packet.

Sorry, I wasn't clear. Let me try to be as clear as possible:

Last year a developer submitted an Issue where key presses were not being handled correctly when using a Terminal.Gui app over RDP (gui-cs#2008). In investigating that issue you and @tznind learned about ConsoleKey.Packet.

We learned that ConsoleKey.Packet meant:

Used to pass Unicode characters as if they were keystrokes. The VK_PACKET key is the low word of a 32-bit Virtual Key value used for non-keyboard input methods. For more information, see Remark in KEYBDINPUT, SendInput, WM_KEYDOWN, and WM_KEYUP"

The only OS that knows what VK_PACKET is, is Windows.

In the fix (gui-cs#2032) a whole bunch of infrastructure code was introduced to try to decode these packets to solve for this use-case (users using Terminal.Gui apps over RDP).

In addition to the infrastructure code (ConsoleKeyMapping.DecodeVKPacketToKConsoleKeyInfo etc...) a Scenario was developed to try to simulate things so this code could be better tested (VkeyPacketSimulator).

As far as I can tell, none of the core Terminal.Gui maintainers (myself, you, and @tznind) regularly test the VK_PACKET support in a real scenario (e.g. over RDP).

The use-case we are discussing is completely tied to Windows. The only way a ConsoleDriver will ever get a key event with ConsoleKey.Packet is if the user is running their app on Windows and using that app over RDP (there may be other cases but nobody's articulated one precisely). The only ConsoleDrivers that will ever get a key event with ConsoleKey.Packet are WindowsDriver and NetDriver.
The infrastructure code discussed above is inherently flawed. Specifically: HashSet<ScanCodeMapping> _scanCodes. It assumes it is possible to have a singular map of scancodes, virtual key codes (VK_), and shift modifiers to characters that works across all keyboard layouts. It, effectively, tries to duplicate what the Win32 method MapVirtualKeyCode does in a very simplistic way (that will never work).
By extension to 2) above, the VkeyPacketSimulator is inherently flawed and doesn't do what it was intended to do (provide a way of testing VK_PACKET handling in WindowsDriver and NetDriver).
Not directly related to VK_PACKET support, but relevant is the fact that WindowsDriver and NetDriver have always been flawed in how they handled keys like Oem3. The only correct way to handle those keys is to use MapVirtualKeyCode (this PR addresses this).

The correct solution to all of this is:

a) Create clear and easy to use instructions for setting up a system to routinely test Terminal.Gui over RDP (and any other use-case where VK_PACKET key events are generated).
b) Change WindowsDriver and NetDriver to use MapVirtualKeyCode when key events with VK_PACKET are received. Most of the code in ConsoleKeyMapping can be thrown away. I'm partially through making this change.
c) Built a better testing API into ConsoleDriver that lets us simulate VK_PACKET key events in unit tests such that both WindowsDriver and NetDriver are better tested.
d) Refactor the VkeyPacketSimulator to use c), so we can have a UI-based tester.

I have a bad memory, and I apologize this didn't come to me before. But in 2004-ish I was partially responsible for adding VK_PACKET support to Windows. I had created the concept of Windows Media Center Extenders (Bobsled) and we were in deep collaboration with the RDP team because I had the vision that Extenders would use RDP as their protocol. All UI could run on the Media Center PC an be remoted to the Bobsled device as GDI primitives via RDP. We needed two features RDP didn't have at the time:

The ability to have video streams be sent to the RDP client out-of-band of the GDI streaming.
The ability to have commands from the IR-based MCE remote control be sent out-of-band of the GDI streaming.

The RDP team had woken up to the fact that RDP didn't support Unicode and varying keyboard layouts well. Hence VK_PACKET was created (and the other Media Center specific VK_ keys such as VK_MEDIA_NEXT_TRACK.

I am pretty sure I'm still confused on some details of all of this (e.g. the whole AltGr thing), but I'm very confident I understand the architectural issues, and I'm 100% sure that trying to build Windows virtual key to char mapping without asking Windows (MapVirtualKeyCode) for help will never scale.

I hope that there's a similar API to MapVirtualKeyCode (or GetKeyboardLayout) on the Mac and Linux. There has to be. And it's possible the keyboard coding used maps well to a lot of Windows VK_ defines. There may even be a key event that is similar to VK_PACKET where all you get is the unicode char, which would be sweet. But that is a separate discussion.

tig · 2024-01-02T16:17:05Z

Found this (which I haven't read yet, but wanted to document it):

https://wiki.archlinux.org/title/Keyboard_input

Note /usr/include/linux/input-event-codes.h is where Linux key codes are defined. There is no equivalent to VK_PACKET I can see.

BDisp · 2024-01-02T16:19:32Z

In Linux I really add code to test only the Simulator.

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs

BDisp · 2024-01-02T15:42:10Z

Terminal.Gui/ConsoleDrivers/ConsoleKeyMapping.cs

+		new (13, VK.OEM_6, 0, '+'), // Oem6
+		new (13, VK.OEM_6, ConsoleModifiers.Shift, '*'),


In POR layout Oem6 is « and with shift is ». I doubt this is the right way. So you made changes to get the right values for your keyboard layout but broken the POR layout.

I can't send to the second TextView on the Simulator |!"#$%&/()=?»>;:_.

Not sure what's going on here, but if I enable the "POR" keyboard and press the Shift-Oem6 key (labeled =/+ on the ENG keyboard) I see this:

event: [KeyEventRecord(down,1,OEM_6,13,»,ShiftPressed)] [ConsoleKeyInfoEx(Key: 221 (Oem6) | Shift, KeyChar: » (187))] event: [KeyEventRecord(up,1,OEM_6,13,»,ShiftPressed)] [ConsoleKeyInfoEx(Key: 221 (Oem6) | Shift, KeyChar: » (187))]

This is the correct behavior.

I can't send to the second TextView on the Simulator |!"#$%&/()=?»>;:_.

At this point, I do not care about the simulator. It serves no purpose right now other than to be a distraction. It doesn't actually simulate anything important. I am focused on fixing keyboard handling in a keyboard layout-independent way. That has nothing to with VK_PACKET nor the simulator.

These are the values. You changed many of these and you must check it it's all right. It appears as OEM_6 because you configured with that value on the scan code equal to 13.

Not sure what's going on here, but if I enable the "POR" keyboard and press the Shift-Oem6 key (labeled =/+ on the ENG keyboard) I see this:

Mine is labeled »/« on POR keyboard
.

I think this is going to be a lot of work, like trying to resurrect a dead person. If you think it's the correct behavior because on the POR layout of your keyboard is right, then we cannot have an agreement here because on mine it isn't correct.

If I use the ENG layout I get the OemPlus which is the correct behavior. So something is wrong.

I just pushed a quick change that will print the keyboard layout name on each keypress.

Note that GetKeybaordLayoutName uses the current threadID and thus will only report the correct name if you did NOT change the keyboard layout after the debugging session was started.

Please

Set your keyboard layout to POR

debug UICatalog

Press the »/« key

Send what you see in the debug output. I see:

event: KBD: 00000816 [KeyEventRecord(down,1,OEM_6,13,«,0)] [ConsoleKeyInfoEx(Key: 221 (Oem6), KeyChar: « (171))] event: KBD: 00000816 [KeyEventRecord(up,1,OEM_6,13,«,0)] [ConsoleKeyInfoEx(Key: 221 (Oem6), KeyChar: « (171))]

If I re-run the above, but first set my keyboard layout to ENG I see:

event: KBD: 00000409 [KeyEventRecord(down,1,OEM_PLUS,13,=,0)] [ConsoleKeyInfoEx(Key: 187 (OemPlus), KeyChar: = (61))] event: KBD: 00000409 [KeyEventRecord(up,1,OEM_PLUS,13,=,0)] [ConsoleKeyInfoEx(Key: 187 (OemPlus), KeyChar: = (61))]

Both of these are correct.

It's the same as your.

event: KBD: 00000816 [KeyEventRecord(down,1,OEM_6,13,«,0)] [ConsoleKeyInfoEx(Key: 221 (Oem6), KeyChar: « (171))] event: KBD: 00000816 [KeyEventRecord(up,1,OEM_6,13,«,0)] [ConsoleKeyInfoEx(Key: 221 (Oem6), KeyChar: « (171))]

event: KBD: 00000409 [KeyEventRecord(down,1,OEM_PLUS,13,=,0)] [ConsoleKeyInfoEx(Key: 187 (OemPlus), KeyChar: = (61))] event: KBD: 00000409 [KeyEventRecord(up,1,OEM_PLUS,13,=,0)] [ConsoleKeyInfoEx(Key: 187 (OemPlus), KeyChar: = (61))]

Good. So it's all working correctly now. Yay!

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs

BDisp · 2024-01-02T22:56:05Z

@tig do you want I merge this now?

tig · 2024-01-03T02:09:24Z

Feel free to merge my pr into your pr. But this is not ready to merge to v2_develop.

If you merge into your pr, I'll keep doing more PRs to yours.

tig · 2024-01-03T04:24:36Z

I thought I was going crazy.

Everything was working so well.

Then I tried testing Ctrl+\ and Ctrl+Shift+\ (Oem6).

No matter what I did, I couldn't get a KeyDown for Ctrl+\.

After pulling my hair out, I discovered that ShareX is somehow intercepting Ctrl+\.

Sheesh.

tig · 2024-01-03T07:04:29Z

I have been unable to get any key events with inputEvent.KeyEvent.wVirtualKeyCode == (VK)ConsoleKey.Packet via RDP.

Until we can get a setup that actually causes the code in FromVKPacketToKeyEventRecord to be called for real, we should just ignore all the related code and the VkeySimulator scenario.

tig · 2024-01-03T07:33:03Z

@BDisp Windows driver seems to be working well now.

Next we need to tweak NetDriver to do the same. Right now it's pretty broken.

BDisp · 2024-01-03T13:24:18Z

Feel free to merge my pr into your pr. But this is not ready to merge to v2_develop.

If you merge into your pr, I'll keep doing more PRs to yours.

So I'll not merge it yet until you think it are ready to merge.

tig · 2024-01-03T15:28:22Z

Are you wiling to help with NetDriver? If so, merge now?

If not, I'll try to get to NetDriver asap.

BDisp · 2024-01-03T15:32:23Z

I think you are in the right way and I would prefer to wait to see what is yours idea for this, if you don't mind. Thanks.

BDisp · 2024-01-03T21:50:06Z

@tig I don't know if this can help to play with VK_PACKET.
https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-keybdinput

This is in Python:
https://gist.github.com/wgm89/1c2c0d3ff50f63df975a

This is in C#:
https://gist.github.com/pedoc/4126e8716667bdb46d4989cddffef2e6

tig · 2024-01-03T23:02:38Z

@tig I don't know if this can help to play with VK_PACKET. https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-keybdinput

This is in Python: https://gist.github.com/wgm89/1c2c0d3ff50f63df975a

This is in C#: https://gist.github.com/pedoc/4126e8716667bdb46d4989cddffef2e6

None of these are "real world" setups. They are all simulations.

If we want more simulated/forced scenarios, we could also use my own Media Center Controller which uses SendInput for the CharsCommand and SendInputCommands.

My latest research indicates that the VK_PACKET code gets lit up when using non-Windows RDP clients (like the one on an iPad) or AnyDesk.

tig · 2024-01-04T00:06:47Z

@BDisp have you ever seen NetDriver behave completely differently under WSL vs. Windows in regards to what key events it gets?

BDisp · 2024-01-04T00:10:25Z

I think so. What differences do you are getting?

tig · 2024-01-04T00:28:02Z

I think so. What differences do you are getting?

As an example: Ctrl+7 works fine with -usc under WSL.

On Windows it's broken.

"Ctrl+8" gets "Backspace":

I just tested develop and v2_develop and the same thing happens there.

BDisp · 2024-01-04T00:30:33Z

Ok I will see that tomorrow.

tig · 2024-01-04T00:32:37Z

Ok I will see that tomorrow.

Related, I think: https://devblogs.microsoft.com/dotnet/console-readkey-improvements-in-net-7/

BDisp · 2024-01-04T00:45:30Z

It seems Net80 complicates it even more.

BDisp · 2024-01-04T15:59:13Z

Sorry @tig but Console.ReadKey is very buggy on both but worse on Windows. It worked much better before. The dotnet guys are screwing this up using virtual terminal. I'm not even try to fix this because isn't Terminal.Gui issue. Not returning nothing because a key binding is consumed by the OS is normal, but returning wrong values is bug and without sense because the values can't even be mapped. I prefer to receive the values as ANSI escape and decode them, than getting very buggy values.

tig · 2024-01-04T16:02:30Z

Sorry @tig but Console.ReadKey is very buggy on both but worse on Windows. It worked much better before. The dotnet guys are screwing this up using virtual terminal. I'm not even try to fix this because isn't Terminal.Gui issue. Not returning nothing because a key binding is consumed by the OS is normal, but returning wrong values is bug and without sense because the values can't even be mapped. I prefer to receive the values as ANSI escape and decode them, than getting very buggy values.

dotnet/runtime#96490

tig · 2024-01-04T16:32:43Z

This makes me happy: VS2022's new Attach to Process capability actually works well for WSL finally!

My plan is to get NetDriver working as well as I can under WSL and ignore Windows for now. It's close.

BDisp · 2024-01-04T17:48:09Z

This makes me happy: VS2022's new Attach to Process capability actually works well for WSL finally!

@tig but this always worked well with the Attach to Process. What profile you are using? The only issue with VS2022 isn't providing debug WSL directly on a console by pressing F5. What I'm missing?

BDisp · 2024-01-04T17:57:17Z

Sorry for I misunderstood badly and merge this, but I already saw the #173.

tig · 2024-01-04T17:59:01Z

Sorry for I misunderstood badly and merge this, but I already saw the #173.

All good. Back on track.

BDisp · 2024-01-04T18:04:50Z

This makes me happy: VS2022's new Attach to Process capability actually works well for WSL finally!

@tig but this always worked well with the Attach to Process. What profile you are using? The only issue with VS2022 isn't providing debug WSL directly on a console by pressing F5. What I'm missing?

Unless you are using a preview version with a new feature.

tig · 2024-01-04T19:10:01Z

This makes me happy: VS2022's new Attach to Process capability actually works well for WSL finally!

@tig but this always worked well with the Attach to Process. What profile you are using? The only issue with VS2022 isn't providing debug WSL directly on a console by pressing F5. What I'm missing?

Unless you are using a preview version with a new feature.

Older versions would never connect for me. I'm running Version 17.9.0 Preview 2.1

tig · 2024-01-09T13:40:01Z

@BDisp - see this: dotnet/runtime#96490 (comment)

I just did a test:

I commented out

		if (p == PlatformID.Win32NT || p == PlatformID.Win32S || p == PlatformID.Win32Windows) {
			//IsWinPlatform = true;
			//try {
			//	NetWinConsole = new NetWinVTConsole ();
			//} catch (ApplicationException) {
			//	// Likely running as a unit test, or in a non-interactive session.
			//}
		}

Theoretically, this should have made Windows work just like WSL. And for KEYBOARD it does!

To debug further, based on Adam's question, I tried this:

		if ((mode & ENABLE_VIRTUAL_TERMINAL_INPUT) < ENABLE_VIRTUAL_TERMINAL_INPUT) {
			//mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
			if (!SetConsoleMode (_inputHandle, mode)) {
				throw new ApplicationException ($"Failed to set input console mode, error code: {GetLastError ()}.");
			}
		}

By not enabling ENABLE_VIRTUAL_TERMINAL_INPUT the keyboard problem on Windows is fixed too.

But, mouse no longer works.

Do you know why Mouse does not work on Windows with NetDriver but does on WSL with native System.Console without all the ENABLE_VIRTUAL_TERMINAL_INPUT stuff?

Before I reply to Adam, I'd like to understand the history better. Not sure if you know or not. Thanks.

BDisp · 2024-01-09T14:19:05Z

@tig - Without the ENABLE_VIRTUAL_TERMINAL_INPUT on Windows the mouse doesn't work because Console.ReadKey will only return the keyboard keys and not mouse clicks/move. With ENABLE_VIRTUAL_TERMINAL_INPUT then the Console.ReadKey will return all the ANSI escape sequences when the keyboard and mouse are used.

Ansi parser net driver

tznind and others added 12 commits December 31, 2023 05:44

Add ToString implementation for SliderOption<T> (gui-cs#3103)

55bf533

* Add ToString implementation for SliderOption<T> * Add test case for SliderOption ToString with more complex class

Add constructor overload to SliderOption<T> (gui-cs#3101)

8954fa5

* Add constructor overload to SliderOption * Adjust whitespace * Move tests to SliderOptionTests * Empty commit for CI

Merge branch 'v2_vkeypacketsimulator-fix_3054' of github.com:BDisp/Te…

afcdb7a

…rminal.Gui into BDisp-v2_vkeypacketsimulator-fix_3054

Updated ScanCodeMapping table with readable constants

cf684ed

Documented bugs

4db8ce3

Implemented mapping using MapVirtualKeyEx

f5c92b9

Implemented mapping using MapVirtualKeyEx

5f0c035

Changed KeyCode special keys to match ConsoleKey values + max unicode…

9cd85fc

… codepoint

Fixed bogus CollectionNavigator impl and tests

8893ffe

Nuked DeleteChar. renamed InsertChar to Insert

466914a

KeyCode.Enter = ConsoleKey.Enter, not \n

1708762

Code cleanup

da71023

BDisp reviewed Jan 2, 2024

View reviewed changes

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs Outdated Show resolved Hide resolved

BDisp reviewed Jan 2, 2024

View reviewed changes

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs Outdated Show resolved Hide resolved

BDisp reviewed Jan 2, 2024

View reviewed changes

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs Outdated Show resolved Hide resolved

BDisp reviewed Jan 2, 2024

View reviewed changes

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs Outdated Show resolved Hide resolved

Terminal.Gui/ConsoleDrivers/WindowsDriver.cs Outdated Show resolved Hide resolved

tig added 2 commits January 2, 2024 14:27

Added diag for keyboard layout name

f817bc7

Fixed AltGr support (hopefully)

5e94f72

tig added 2 commits January 2, 2024 23:27

Simplified code

64ec76d

Simplified KeyCode by removing ShiftKeys

9d3de85

tig added 2 commits January 3, 2024 00:33

Code cleanup

ce8db7e

Fixes cursesdriver (somewhat)

85c8d3d

Code cleanup

198e261

tig mentioned this pull request Jan 4, 2024

Fixes #2465 - Simplify/Enforce LayoutStyle.Absolute when X, Y, Width, and/or Height are null or Absolute() gui-cs/Terminal.Gui#2467

Merged

BDisp merged commit b7fca27 into BDisp:v2_vkeypacketsimulator-fix_3054 Jan 4, 2024

tig deleted the BDisp-v2_vkeypacketsimulator-fix_3054 branch April 3, 2024 01:47

BDisp pushed a commit that referenced this pull request Dec 7, 2024

Merge pull request #172 from tznind/ansi-parser-net-driver

9aff491

Ansi parser net driver

		new (13, VK.OEM_6, 0, '+'), // Oem6
		new (13, VK.OEM_6, ConsoleModifiers.Shift, '*'),

Fixes key handling for Windows. #172

Fixes key handling for Windows. #172

Conversation

tig commented Jan 2, 2024

BDisp commented Jan 2, 2024 • edited Loading

tig commented Jan 2, 2024 • edited Loading

BDisp commented Jan 2, 2024

tig commented Jan 2, 2024

tig commented Jan 2, 2024

BDisp commented Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tig Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BDisp commented Jan 2, 2024

tig commented Jan 3, 2024

tig commented Jan 3, 2024

tig commented Jan 3, 2024

tig commented Jan 3, 2024

BDisp commented Jan 3, 2024

tig commented Jan 3, 2024

BDisp commented Jan 3, 2024

BDisp commented Jan 3, 2024 • edited Loading

tig commented Jan 3, 2024

tig commented Jan 4, 2024

BDisp commented Jan 4, 2024

tig commented Jan 4, 2024

BDisp commented Jan 4, 2024

tig commented Jan 4, 2024

BDisp commented Jan 4, 2024

BDisp commented Jan 4, 2024

tig commented Jan 4, 2024

tig commented Jan 4, 2024

BDisp commented Jan 4, 2024

BDisp commented Jan 4, 2024

tig commented Jan 4, 2024

BDisp commented Jan 4, 2024

tig commented Jan 4, 2024

tig commented Jan 9, 2024

BDisp commented Jan 9, 2024 • edited Loading

BDisp commented Jan 2, 2024 •

edited

Loading

tig commented Jan 2, 2024 •

edited

Loading

BDisp commented Jan 2, 2024 •

edited

Loading

tig Jan 2, 2024 •

edited

Loading

BDisp commented Jan 3, 2024 •

edited

Loading

BDisp commented Jan 9, 2024 •

edited

Loading