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

Fix #5784: Key bindings won't consume dead keys #7686

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Fix #5784: Key bindings won't consume dead keys #7686

merged 1 commit into from
Oct 19, 2020

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 20, 2020

Summary of the Pull Request

Let's assume the user has bound the dead key ^ to a sendInput command that sends "b".
If the user presses the two keys ^a it'll produce "bâ", despite us marking the key event as handled.
We can use ToUnicodeEx to clear such dead keys from the keyboard state and should make use of that for keybindings.
Unfortunately SetKeyboardState cannot be used for this purpose as it doesn't clear the dead key state.

PR Checklist

  • Closes Ability to disable some dead keys #5784
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

  • Enabled a German keyboard layout
  • Added the following two keybindings:
    { "command": { "action": "sendInput", "input": "x" }, "keys": "q" },
    { "command": { "action": "sendInput", "input": "b" }, "keys": "^" }
  • Pressed the following keys → and ensured that the given text is printed in that order:
    • q → x
    • ´ → nothing
    • a → á
    • ^ → b
    • a → a (previously this would print: â)
    • ´ → nothing
    • ^ → b
    • a → a (unfortunately we cannot specifically clear only ^)

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 20, 2020
@zadjii-msft zadjii-msft self-assigned this Oct 15, 2020
@miniksa
Copy link
Member

miniksa commented Oct 16, 2020

Wow this is an incredibly creative way of tinkering with the tables inside user32. Props to the guy who discovered it.

I did go read behind SetKeyboardState and it is definitely inadequate for the dead key scenario relative to what happens through ToUnicodeEx. The former only updates some bit flags related to things being up/down. The latter does that AND goes through the whole dance of manipulating Alt+Numpad entry state, dead key cached characters, and more.

To be clear, both of these functions are updating a thread specific table about keyboard state. It's not strictly global. It's just that SetKeyboardState looks at and updates just one specific member of the state table while ToUnicodeEx roots around in pretty much every bit of it.

Why there isn't a more adequate way of doing this is likely lost to the sands of time. If this does the job, I'm perfectly OK with it.

// http://archives.miloush.net/michkap/archive/2006/09/10/748775.html
// > "The key here is to keep trying to pass stuff to ToUnicode until -1 is not returned."
std::array<wchar_t, 16> buffer;
while (ToUnicodeEx(vkey, scanCode, keyState.data(), buffer.data(), gsl::narrow_cast<int>(buffer.size()), 0b1, nullptr) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, the nullptr in the final field here in the HKL position is what is causing the internals behind ToUnicodeEx to look up and modify the state in the thread.

@lhecker
Copy link
Member Author

lhecker commented Oct 16, 2020

Thanks for the detailed explanation! I've updated the PR message above. 👍

@miniksa
Copy link
Member

miniksa commented Oct 16, 2020

Wow this is an incredibly creative way of tinkering with the tables inside user32. Props to the guy who discovered it.

Oh. That link is to an archive of Michael S. Kaplan posts who I believe worked at Microsoft behind this stuff. So that really explains why it's the right answer then, huh? :)

@zadjii-msft zadjii-msft removed their assignment Oct 19, 2020
@DHowett
Copy link
Member

DHowett commented Oct 19, 2020

I'd like to have a look at this one before y'all merge it. I'll do that today.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I am deeply horrified that this is what's necessary, but I completely believe it.

@DHowett DHowett changed the title Fixed #5784: Key bindings won't consume dead keys Fix #5784: Key bindings won't consume dead keys Oct 19, 2020
@DHowett DHowett merged commit 4099aac into microsoft:master Oct 19, 2020
@DHowett
Copy link
Member

DHowett commented Oct 19, 2020

Thank you so much for doing this.

DHowett pushed a commit that referenced this pull request Oct 27, 2020
Let's assume the user has bound the dead key ^ to a sendInput command
that sends "b".  If the user presses the two keys ^a it'll produce "bâ",
despite us marking the key event as handled.  We can use `ToUnicodeEx`
to clear such dead keys from the keyboard state and should make use of
that for keybindings.  Unfortunately `SetKeyboardState` cannot be used
for this purpose as it doesn't clear the dead key state.

Validation
* Enabled a German keyboard layout
* Added the following two keybindings:
  { "command": { "action": "sendInput", "input": "x" }, "keys": "q" },
  { "command": { "action": "sendInput", "input": "b" }, "keys": "^" }
* Pressed the following keys → ensured that the given text is printed:
  * q → x
  * ´ → nothing
  * a → á
  * ^ → b
  * a → a (previously this would print: â)
  * ´ → nothing
  * ^ → b
  * a → a (unfortunately we cannot specifically clear only ^)

Closes #5784

(cherry picked from commit 4099aac)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Nov 11, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

@lhecker lhecker deleted the fix-5784-dead-key-send-input branch November 11, 2020 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to disable some dead keys
4 participants