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

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

Merged
merged 38 commits into from
Jan 4, 2024

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Dec 24, 2023

Fixes a lot of keyboard related things, but does NOT fix #3054

Pull Request checklist:

  • [c] I've named my PR in the form of "Fixes #issue. Terse description."
  • [c] 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 BDisp requested a review from tig as a code owner December 24, 2023 01:23
@BDisp
Copy link
Collaborator Author

BDisp commented Dec 27, 2023

@tig have you at least looked at this PR to understand the reason for the changes?

@tig
Copy link
Collaborator

tig commented Dec 27, 2023

No, I've not had a chance. But I will asap.

@tig
Copy link
Collaborator

tig commented Dec 28, 2023

What is the setup required to actually get the VK_PACKET stuff
to come into play?

Is it an Telnet/ssh thing. ?

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 28, 2023

What is the setup required to actually get the VK_PACKET stuff to come into play?

Is it an Telnet/ssh thing. ?

Yes, know it's used with RDP (Remote Desktop Protocol) where the unicode char is encoded with the virtual key.

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 28, 2023

Typing in the first TextView mirrors all on the second TextView on the VK Packet Simulator. All the key bindings that aren't supported by the TextView will open the MessageBox with the the typed key binding information.

@tig
Copy link
Collaborator

tig commented Dec 29, 2023

What is the setup required to actually get the VK_PACKET stuff to come into play?
Is it an Telnet/ssh thing. ?

Yes, know it's used with RDP (Remote Desktop Protocol) where the unicode char is encoded with the virtual key.

I understand what the simulator is supposed to do.

I want to be able to use VK Packet support "for real". Do you know what I need to do in order to do that?

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 29, 2023

What is the setup required to actually get the VK_PACKET stuff to come into play?
Is it an Telnet/ssh thing. ?

Yes, know it's used with RDP (Remote Desktop Protocol) where the unicode char is encoded with the virtual key.

I understand what the simulator is supposed to do.

I want to be able to use VK Packet support "for real". Do you know what I need to do in order to do that?

I never try it with a real support but here (#2008 (comment)) is a way to try it.

@tig
Copy link
Collaborator

tig commented Dec 29, 2023

@BDisp, you wrote this:

image

You are not wrong in saying "'â' is the lower case of 'Â`". However, in all of my testing, on the US ENG, DEU, POR, and other keyboards this is NOT how keyboards work, or what users expect.

For example, in either Windows Terminal settings, use the settings UI to change the keybinding for "New tab, proifle index 0"

image

Using the DEU keyboard, the "CTRL+SHIFT+ä" key (which in ENG is "CTRL+SHIFT+'" gets displayed as "Ctrl+Shift+ä" NOT "Ctrl-Shift+Ä"

The same is true for, eg.., the Key.D7.

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 29, 2023

@BDisp, you wrote this:

image You are not wrong in saying "'â' is the lower case of 'Â`". However, in all of my testing, on the US ENG, DEU, POR, and other keyboards this is NOT how keyboards work, or what users expect.

For example, in either Windows Terminal settings, use the settings UI to change the keybinding for "New tab, proifle index 0"

image Using the DEU keyboard, the "CTRL+SHIFT+ä" key (which in ENG is "CTRL+SHIFT+'" gets displayed as "Ctrl+Shift+ä" NOT "Ctrl-Shift+Ä"

The same is true for, eg.., the Key.D7.

@tig I have a Portuguese keyboard and I know what I'm talking about. To get the â I need type shift+^+A but the shift modifier doesn't matter because it's only to get the ^. After type the A the result is â (remember that the A without shift represent the lower a).

image

To get the  I need to type shift+^+shift+A , but the shift modifier doesn't matter because it's only to get the ^ but the second shift modifier to get the upper case A matter. After type A the result is Â.

image

Using a virtual keyboard isn't the same as a real keyboard. So a Ctrl+Shift+â is never possible because other modifier is been used, the Ctrl key. Only is possible Ctrl+Shift+A.
If I type Ctrl+Shift+/ I get the follow on WindowsDriver and it must represented as KeyCode.CtrlMask | KeyCode.ShiftMask | KeyCode.D7 because other modifier is been used, the Ctrl.

image

If I type Shift+/ I get the follow and we could consider as KeyCode.ShiftMask | (KeyCode)'/'

image

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 30, 2023

My keyboard has more or less this layout:

image

Pressing Ctrl+Shift+~ in WT the result is ctrl+shift+~. Although I have the shift key pressed it's ignored if another modifier key, like the Ctrl or Alt, are also pressed. So only the lower character is considered (~).

image

Pressing Shift+~ in WT the result is shift+~. WT only consider the lower character (~) which isn't the right binding.

image

If I press Shift+~ in the Windows notepad I get nothing and only when I press Shift+~ again I get two ^^. This is normally for accented characters because the terminal is waiting for the next key pressed to write the right one. If I press Shift+~ and release and then press a I get â. If I press Shift+~ and release and then press Shift+a I get Â. This last normally we don't need to release the keys and is done at once, like Shift+~+a or Shift+~ and without release the shift press a.

image

Pressing Ctrl+Shift+7 in WT the result is ctrl+shift+7. Although I have the shift key pressed it's ignored if another modifier key, like the Ctrl or Alt, are also pressed. So only the lower character is considered (7).

image

Pressing Shift+7 in WT the result is shift+7. WT only consider the lower character (7) which isn't the right binding.

image

If I press Shift+7 in the Windows notepad I get /. This is right behavior. So, don't rely on the WT to test the right key bindings because we need keys that interpret the right writings as well. It's better to use the notepad to test the printable keys.

image

@tig
Copy link
Collaborator

tig commented Dec 31, 2023

@BDisp please read this: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-mapvirtualkeya

Esp:

image

This is the mode I believe Terminal.Gui should operate in.

The current code in this PR (and v2_develop) is broken w.r.t. keys like OemPlus and more. Neither of you nor I had this correct.

The _scanCodes HashSet is very broken (and has been since it was introduced). As an example:

image

This can be demonstrated in v1 (develop) using the ENG keyboard and trying to type @ (which is Shift-D2):
image

Since you use POR, you do not see this because you coded all this to assume POR. With the POR keyboard:
image

Likewise, ConsoleKeyMappingTests.GetConsoleKeyInfoFromKeyCode_Tests is (and has always been) broken:

image

This is broken too:

image

In short, your understanding of how VK_PACKET works is incorrect. Previously, I was confused too (in a different way).

I am currently working on this and will submit a PR to PR with my suggested changes in the next few days. Please work on something else in the meantime so we are not working against each other.

@BDisp
Copy link
Collaborator Author

BDisp commented Jan 1, 2024

This is bad in my opinion. Currently KeyCode.Delete=127 which represent the ASCII 127 (DEL) and the KeyCode.DeleteChar is a masked value that really represent the ConsoleKey.Delete=46, which represent the ASCII . and thus must be a masked value. In my opinion we should remove the KeyCode.Delete and replace the KeyCode.DeleteChar with the KeyCode.Delete, because we don't need the ASCII 127 represented as KeyCode, I think.
And the KeyCode.InsertChar should be replaced with KeyCode.Insert because ConsoleKey.Insert=45, which represent the ASCII - and we only must use the masked value for KeyCode.Insert. Do you agree?

@dodexahedron
Copy link
Collaborator

I think @BDisp is encountering what I was ultimately getting at but not communicating properly. 😅

Isn't globalization fun?

@tig
Copy link
Collaborator

tig commented Jan 4, 2024

@BDisp do you know what this is?

image

@BDisp
Copy link
Collaborator Author

BDisp commented Jan 4, 2024

@BDisp do you know what this is?

image

I think I used that a long time ago to use on the Application class to get some request, like the terminal type, but it mustn't work like that. Can be removed.

@tig tig changed the title Fixes #3054. VkeyPacketSimulator scenario is broken. Cleans up key handling in drivers (was "fixes VkeyPacketSimulator is broken") Jan 4, 2024
tig and others added 2 commits January 4, 2024 12:25
@tig tig merged commit 0484fc8 into gui-cs:v2_develop Jan 4, 2024
1 check passed
@BDisp BDisp deleted the v2_vkeypacketsimulator-fix_3054 branch January 4, 2024 19:56
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.

3 participants