Delegate all character input to the character event handler #4192

lhecker · 2020-01-12T19:16:06Z

My basic idea was that WM_CHAR is just the better WM_KEYDOWN.
The latter fails to properly support common dead key sequences like in
#3516.

As such I added some logic to Terminal::SendKeyEvent to make it return
false if the pressed key represents a printable character.
This causes us to receive a character event with a (hopefully) correctly
composed code unit, which then gets sent to Terminal::SendCharEvent.
Terminal::SendCharEvent in turn had to be modified to support
potentially pressed modifier keys, since Terminal::SendKeyEvent isn't
doing that for us anymore.
Lastly TerminalInput had to be modified heavily to support character
events with modifier key states. In order to do so I merged its
HandleKey and HandleChar methods into a single one, that now handles
both cases.
Since key events will now contain character data and character events
key codes the decision logic in TerminalInput::HandleKey had to be
rewritten.

PR Checklist

CLA signed
Tests added/passed
I've discussed this with core contributors already.

Validation Steps Performed

See Terminal does not handle custom keyboard layouts with dead-keys mapping to themselves #3516.
I don't have any keyboard that generates surrogate characters. Due to
this I modified TermControl::_SendPastedTextToConnection to send the
data to _terminal->SendCharEvent() instead. I then pasted the test
string ""𐐌𐐜𐐬" and ensured that the new TerminalInput::_SendChar
method still correctly assembles surrogate pairs.

Closes #3516
Closes #3554 (obsoleted by this PR)
Potentially impacts #391, which sounds like a duplicate of #3516

lhecker · 2020-01-13T00:14:29Z

I'm almost done fixing all tests. Only InputTest::TerminalInputModifierKeyTests is still giving me troubles. (...because HandleKey() is now capable of handling all characters, which this test doesn't know about yet.)

lhecker · 2020-02-02T17:22:06Z

OMG! All checks have passed?! I almost can't even believe it anymore. 😄
This PR was seriously hard to create (due to all the little details surrounding it).
TBH I hope we can merge at least parts of it. 😅

zadjii-msft · 2020-02-28T14:43:29Z

I've owed you a review for far too long on this particular PR. My schedule just became much more available, so I'm going to try and get to this today or Monday. Sorry for the delay!

lhecker · 2020-02-28T17:31:24Z

@zadjii-msft You don’t owe me anything. 🙂
I believe this PR is rather tricky as I decided to completely rewrite the entire input handling logic.
As such please feel free to "rip and tear" my PR apart. (TBH even.)

zadjii-msft · 2020-02-28T17:41:46Z

I absolutely do - IMO, it's our responsibility as project maintainers to reply to good contributions like this in a reasonably prompt timeframe. If the community is going to be generous enough to contribute to us, then the least we owe is the time to review the contribution. That's at least my opinion on that matter.

Besides, I wrote all this code originally like 3 years ago, so it's probably my specific responsibility to review you ripping and tearing this apart 😜

lhecker · 2020-03-14T15:46:15Z

FYI I rebased this PR to resolve merge conflicts that have popped up.

lhecker · 2020-03-17T21:29:48Z

@zadjii-msft Do you happen to have time for some unicode love? 🙈🙈🙈
The last build failed due to a timeout. Can you restart it?

zadjii-msft

Okay major apologies on still delaying this after I had just said that we owed you a review 😅. There was some discussion on whether or not this would make the 1.0 cut or not - initially we were worried that such a major change to our core input handling wouldn't be safe enough, and we wouldn't have the time to properly vet it. Looking over this, it seems to me like this isn't actually all that major a change. The heaviest change is really just in TerminalInput::HandleKey, but that is mostly just making it more readable. I'm also hoping that this solves #391, though I can't really be sure.

I did find one little bug while playing with this:

Ctrl+Alt+Space seems to send

^[^R     27 0033 0x1b
         18 0022 0x12

Instead of

^[^@   27 0033 0x1b
        0 0000 0x00

(output from showkey -a)

Otherwise, I think this looks good for selfhosting for 1.0. @DHowett-MSFT let's pull this one in for the next build too.

src/terminal/input/terminalInput.cpp

zadjii-msft · 2020-03-30T20:46:05Z

src/terminal/input/terminalInput.cpp

+// - keyEvent - Key event to translate
+// Return Value:
+// - True if the event was handled.
+bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)


@ other reviewers: this change isn't so bad if you open the two versions of this function up in not github. Github makes this diff hard to follow, but it's actually a lot easier to read now.

I'm sorry. 🙈
Most changes to this function where necessary to ensure everything still works when you funnel character events into HandleKey() instead of HandleChar() (which doesn't exist anymore).

At some point I got so confused by the code that I just decided to rewrite this function from scratch.
This also means though that it actually might be best if you review changes to this function by pulling up the current code and comparing it side-by-side with the one in my PR, without viewing the diff.
While the structure is entirely different, you should quickly notice that the logic flow itself is the same as before.
I added comments to each section explaining how I understood the code and why it's necessary.

src/cascadia/TerminalCore/Terminal.cpp

lhecker · 2020-03-30T22:06:11Z

@zadjii-msft Honestly I wouldn't be mad at all if you didn't merge this before 1.0.
In fact as you pointed out yourself this PR introduced a bug. 😄
I'll try to swiftly fix all bugs that I can find though and make sure this PR at least works just as well as the current master.

lhecker · 2020-03-31T20:17:32Z

@zadjii-msft I feel like I'm loosing my mind. 😐
The issue regarding Ctrl+Alt+Space resulting in ^[^R is fixed if you change this condition to be ch >= 0x40 instead.
If you attach a debugger with a breakpoint condition of ch < 0x40 you'll notice that this breakpoint is apparently never hit.
Do you know what's going on here? Is the TerminalInput class being used outside of the debugger's context?

In the meantime I'll change the if condition to reflect the current master branch, because that fixes the issue for whatever reason.

lhecker · 2020-03-31T21:09:19Z

@zadjii-msft I've force-pushed a new version that reduces the amount of changes in this PR by about a quarter. 🙂
It also fixes the Ctrl+Alt+Space issue as mentioned above.
I tested all key combinations that I could think of and made sure that the showkey output is the same between the current master and my version.

lhecker · 2020-03-31T23:17:31Z

F*** Now I remember why I rebuilt the entire inputTest.cpp. 😄
I changed the code to use a std::wstring because there's now a new situation where the buffer can contain e.g. \x1b\x00 and the test should definitely cover that situation.
The current approach with the s_pwsInputBuffer doesn't support this though, because it's C-string based (i.e. null-terminated).

lhecker · 2020-03-31T23:28:33Z

Since the last time you reviewed, @zadjii-msft, this has happened.
I'm extremely sorry for me rebasing this PR. I thought I could get rid of a huge chunk of unnecessary changes, but it turns out I actually made them for a reason. I could've just pushed the changes as separate commits. Sorry. 🙈

If you dislike the size of this PR I could find a way to integrate the changes necessary for #3516 without modifying TerminalInput::HandleKey too much btw. I believe that should be possible.

zadjii-msft · 2020-04-01T13:52:15Z

sorry for me rebasing this PR

Don't worry about it, happens all the time! Providing that delta link is much appreciated :)

If you dislike the size of this PR

lol no, it's perfectly fine. It's just one of those types of changes that github has a particularly hard time with. I couldn't care less about the actual size, looking through the code I'm not terribly scared of it. You're version of TerminalInput::HandleKey looks infinitely better and I'd hate to lose that goodness ❤️

zadjii-msft

Okay so Ctrl+Alt+Space was fixed for conhost, but it looks like it's still broken in the Terminal 😢 Now it's just generating a ^@, a single NUL.

I think the Terminal is synthesizing the right sequence, but maybe conpty is generating the wrong input for it now, or maybe the input that's generated by conpty doesn't get re-translated back to ^[^@ correctly. I can try to keep investigating to figure out where the miscommunication is, if you need help.

Other than this, I think everything looks great

zadjii-msft

I'm investigating more - turns out that ctrl+alt+space thing isn't a regression from this change, conpty just doesn't know how to deal with that right now. So you're off the hook on that one ☺️

lhecker · 2020-04-01T15:19:03Z

@zadjii-msft Yeah I was just about to say that as well.

As I wrote above the most puzzling thing for me though is if you change this if condition (line 510) to a simple ch != 0 - which should be correct as well.
If you attach a debugger you'll see that TerminalInput definitely invokes _pfnWriteEvents with a \x1b\x00 sequence.
But then showkey shows a ^[^R sequence instead. How can this happen? I don't understand how such a benign change to the if condition can cause this. 😞

zadjii-msft · 2020-04-01T15:42:49Z

Okay I maybe saw what happened here.

On a en-us keyboard, '@' is shift+2.
When conpty sees a \x1b\x0, it converts it to [Shift_down, Ctrl_down, Alt_down, 2_down, 2_up, Alt_up, Ctrl_up, Shift_up].
- I did not pay a ton of attention to the keys that got synthesized here. I need to re-investigate the [2_down, 2_up]
When we get back to TerminalInput::HandleKey, we end up skipping that branch, because the ch was 0n50, which is 0x32, which is '2'.

I'm not sure what's exactly the right behavior here. I'd think we probably should send a different key, but I'll need to investigate more. It's possible that the ch != 0 change would work just fine, but I want to make sure that the upstream input from conpty is actually correct.
I've got a braindead fix that would fix showkey -a, but that doesn't really resolve the issue here for Win32 applications unfortunately, so I'm gonna file a separate issue to track this investigation.

…e's a problem in Win32 quite yet. (#5208) ## Summary of the Pull Request When conpty is in VT input mode, we pass through all the input we receive. This includes all the other `Action*Dispatch` methods, but missed this one. ## References * Missed during #4856 * Discovered during the course of the #4192 review * #5205 Also investigated part of the issue, but found a different bug. ## PR Checklist * [x] Doesn't close anything, just related to above things. * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This will fix the <kbd>ctrl+alt+space</kbd> in the Terminal thing mentioned in #4192, but doesn't actually resolve the root cause of that bug (which is tracked in #5205).

DHowett-MSFT

I only found one complaint. This is, for all intents and purposes, almost perfect.

DHowett-MSFT · 2020-04-06T23:23:05Z

src/cascadia/TerminalCore/Terminal.cpp

+    auto vkey = _TakeVirtualKeyFromLastKeyEvent(scanCode);
+    if (vkey == 0 && scanCode != 0)
+    {
+        vkey = _ScanCodeFromVirtualKey(scanCode);


this name worries me. Below, we have VirtualKeyFromCharacter, but here we're assigning vkey but the function name implies it returns a ScanCode

You‘re right: This should be _VirtualKeyFromScanCode. 👍

DHowett-MSFT · 2020-04-06T23:41:39Z

src/terminal/input/terminalInput.cpp

-                // For perf optimization, filter out any typically printable Virtual Keys (e.g. A-Z)
-                // This is in lieu of an O(1) sparse table or other such less-maintainable methods.
-                // VK_CANCEL is an exception and we want to send the associated uChar as is.


Is this a worthwhile optimization to keep? I'd be interested in seeing the real-world performance impact.

_translateDefaultMapping is an extremely simple and fast method (<1us I‘m sure) and I suppose it‘s not necessary to retain such an optimization.
On the other hand the intention of the previous code is easy to understand and doesn’t add much complexity.
I‘ll gladly add it back in. 👍

(Our of curiosity I‘ll make sure to post the performance impact of that function tomorrow. 🙂)

I believe you that it's incredibly minor, so I am okay leaving out the optimization. It used to be a std::map, but then it was replaced with an std::array and not revisited. No need to share performance traces as a requirement for landing this 😄

DHowett-MSFT · 2020-04-07T00:14:38Z

(You'll probably need to merge master to get the spell checking bot going again, but I bet it's going to complain about ecma (instead of ECMA))

ghost · 2020-04-07T19:09:13Z

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett-MSFT

I love this. Thank you.

ghost · 2020-04-22T17:15:15Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

DJm00n · 2022-12-09T21:49:00Z

src/terminal/input/terminalInput.cpp

-            }
+            // For Alt+Ctrl+Key messages GetCharData() returns 0.
+            // -> Get the char from the virtual key.
+            ch = LOWORD(MapVirtualKeyW(keyEvent.GetVirtualKeyCode(), MAPVK_VK_TO_CHAR));


@lhecker this may be broken for non-US layouts.
See https://stackoverflow.com/q/72464583/1795050

lhecker mentioned this pull request Jan 15, 2020

AltGR SEQUENCES DO NOT WORK -- Can't type any AltGr combinations in Windows Terminal #521

Closed

DHowett-MSFT mentioned this pull request Jan 23, 2020

Danish keyboard layout Ctrl+Alt+7 #4279

Closed

lhecker requested a review from zadjii-msft February 7, 2020 16:18

zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Feb 28, 2020

zadjii-msft requested changes Mar 30, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 30, 2020

Delegate all character input to the character event handler

d566d68

zadjii-msft requested changes Apr 1, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 1, 2020

zadjii-msft approved these changes Apr 1, 2020

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 1, 2020

This was referenced Apr 1, 2020

Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205

Closed

Passthrough Alt+C0 sequences in conpty #5208

Merged

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Apr 6, 2020

ghost requested review from miniksa, carlos-zamora, DHowett-MSFT and leonMSFT April 6, 2020 21:20

DHowett-MSFT suggested changes Apr 6, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 6, 2020

Addressed review comments

47f6c42

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 7, 2020

DHowett-MSFT approved these changes Apr 7, 2020

View reviewed changes

ghost merged commit a9c9714 into microsoft:master Apr 7, 2020

DHowett-MSFT mentioned this pull request Apr 7, 2020

Extra character inserted when writing ~ or ` with vscode and french keyboard layout #391

Closed

lhecker deleted the input-handling-fixes branch April 7, 2020 19:43

DHowett-MSFT mentioned this pull request Apr 8, 2020

Dvorak: Ctrl+] may not work correctly over the PTY #4885

Closed

This was referenced Apr 22, 2020

Further refine ToUnicodeEx calls in CharacterFromKeyEvent #3554

Closed

Terminal does not handle custom keyboard layouts with dead-keys mapping to themselves #3516

Closed

lhecker mentioned this pull request Apr 24, 2020

Ctrl+Alt+'char' generates other chars then 'AltGr' (Germany Layout) v0.11.1121.0 #5525

Closed

DJm00n reviewed Dec 9, 2022

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Dec 9, 2022

j4james mentioned this pull request Apr 6, 2023

Send a KeyUp sequence only once a key has been released #15130

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate all character input to the character event handler #4192

Delegate all character input to the character event handler #4192

lhecker commented Jan 12, 2020 •

edited by DHowett-MSFT

Loading

lhecker commented Jan 13, 2020 •

edited

Loading

lhecker commented Feb 2, 2020 •

edited

Loading

zadjii-msft commented Feb 28, 2020

lhecker commented Feb 28, 2020 •

edited

Loading

zadjii-msft commented Feb 28, 2020

lhecker commented Mar 14, 2020

lhecker commented Mar 17, 2020

zadjii-msft left a comment

zadjii-msft Mar 30, 2020

lhecker Mar 30, 2020

lhecker commented Mar 30, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020 •

edited

Loading

zadjii-msft commented Apr 1, 2020

zadjii-msft left a comment

zadjii-msft left a comment

lhecker commented Apr 1, 2020

zadjii-msft commented Apr 1, 2020

DHowett-MSFT left a comment

DHowett-MSFT Apr 6, 2020

lhecker Apr 6, 2020

DHowett-MSFT Apr 6, 2020

lhecker Apr 6, 2020

DHowett-MSFT Apr 7, 2020

DHowett-MSFT commented Apr 7, 2020 •

edited

Loading

ghost commented Apr 7, 2020

DHowett-MSFT left a comment

ghost commented Apr 22, 2020

DJm00n Dec 9, 2022

Delegate all character input to the character event handler #4192

Delegate all character input to the character event handler #4192

Conversation

lhecker commented Jan 12, 2020 • edited by DHowett-MSFT Loading

PR Checklist

Validation Steps Performed

lhecker commented Jan 13, 2020 • edited Loading

lhecker commented Feb 2, 2020 • edited Loading

zadjii-msft commented Feb 28, 2020

lhecker commented Feb 28, 2020 • edited Loading

zadjii-msft commented Feb 28, 2020

lhecker commented Mar 14, 2020

lhecker commented Mar 17, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Mar 30, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020

lhecker commented Mar 31, 2020 • edited Loading

zadjii-msft commented Apr 1, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

lhecker commented Apr 1, 2020

zadjii-msft commented Apr 1, 2020

DHowett-MSFT left a 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

DHowett-MSFT commented Apr 7, 2020 • edited Loading

ghost commented Apr 7, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ghost commented Apr 22, 2020

Choose a reason for hiding this comment

lhecker commented Jan 12, 2020 •

edited by DHowett-MSFT

Loading

lhecker commented Jan 13, 2020 •

edited

Loading

lhecker commented Feb 2, 2020 •

edited

Loading

lhecker commented Feb 28, 2020 •

edited

Loading

lhecker commented Mar 31, 2020 •

edited

Loading

DHowett-MSFT commented Apr 7, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.