Fix VT sequences for Ctrl+Alt+? input #4947

zadjii-msft · 2020-03-16T21:18:36Z

Summary of the Pull Request

Ctrl+/ and Ctrl-? are complicated in VT input.

C-/ is supposed to be ^_ (the C0 character US)
C-? is supposed to be DEL
C-M-/ is supposed to be ^[^_ (ESC US)
C-M-? is supposed to be ^[^? (ESC DEL)

The astute reader will note that these characters also share the same key on a
standard en-us keyboard layout, which makes the problem even more complicated.
This PR does a better job of handling these weird cases.

References

Powershell: Ctrl-Alt-? does not work in Windows Terminal #3079 - At first, I thought this PR would close this issue, but as I've learned below, this won't solve that one. This bug was merely found during that investigation.

PR Checklist

Related to Powershell: Ctrl-Alt-? does not work in Windows Terminal #3079
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

ran tests
checked showkey -a in gnome-terminal, which gives you the wrong results for C-M-/, C-M-?
checked showkey -a in xterm, which gives you the right results for C-M-/, C-M-?
checked showkey -a in conhost
checked showkey -a in Windows Terminal

## Summary of the Pull Request Ctrl+/ and Ctrl-? are complicated in VT input. * C-/ is supposed to be `^_` (the C0 charater US) * C-? is supposed to be `DEL` * C-M-/ is supposed to be `^[^/` * C-M-? is supposed to be `^[?` The astute reader will note that these characters also share the same key on a standard en-us keyboard layout, which makes the problem even more complicated. This PR does a better job of handling these weird cases. ## PR Checklist * [x] Closes #3079 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * ran tests * checked `showkey -a` in gnome-terminal * checked `showkey -a` in conhost * checked `showkey -a` in Windows Terminal

src/terminal/input/terminalInput.cpp

miniksa

Did you check this with an alternate keyboard layout? I hope so. It looks like it will be robust enough to handle it with the VkKeyScan matching.

The only other comment I had was the one Carlos flagged with the typo. So approving.

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

DHowett-MSFT · 2020-03-17T17:43:29Z

src/terminal/input/terminalInput.cpp

+            const bool shift = keyEvent.IsShiftPressed();
+
+            // From the KeyEvent we're translating, synthesize the equivalent VkKeyScan result
+            const short keyScanFromEvent = keyEvent.GetVirtualKeyCode() |


this is quite clever!

should it be a local static function that takes a keyEvent instead? 😄

(i don't think it should be a member on keyevent because that's too win32-specific)

DHowett-MSFT · 2020-03-17T17:43:42Z

src/terminal/input/terminalInput.cpp

+            const bool shift = keyEvent.IsShiftPressed();
+
+            // From the KeyEvent we're translating, synthesize the equivalent VkKeyScan result
+            const short keyScanFromEvent = keyEvent.GetVirtualKeyCode() |


(i don't think it should be a member on keyevent because that's too win32-specific)

src/terminal/input/terminalInput.cpp

j4james · 2020-03-18T01:47:46Z

C-/ is supposed to be ^_ (the C0 charater US)
C-? is supposed to be DEL
C-M-/ is supposed to be ^[^/
C-M-? is supposed to be ^[?

For the record, I think this is wrong. If you look at Mintty, or Xterm (with the meta sends escape mode set), you'll get different results:

Ctl+/ is ^_ (US)
Ctl+? is ^? (DEL)
Ctl+Atl+/ is ^[^_ (ESC US)
Ctl+Atl+? is ^[^? (ESC DEL)
Alt+/ is ^[/ (ESC /)
Alt+? is ^[? (ESC ?)

Notice how it all fits a pattern. Gnome-terminal doesn't distinguish between Ctl+Alt and Alt, which definitely seems like a bug to me.

zadjii-msft · 2020-03-18T15:04:49Z

@j4james great catch! I was using gnome-terminal to test, not xterm. That certainly does make more sense. I'll make sure that roundtrips as nicely through conpty as the other ones did

zadjii-msft · 2020-03-18T15:46:21Z

Gah okay. I concur that the fixed behavior is more correct, but now ctrl+alt+? falls into the #879 category of keys. We can't differentiate between that and Alt+Bksp, but that's probably okay.

…om/Microsoft/Terminal into dev/migrie/b/3079-C-M-questionmark

DHowett-MSFT · 2020-03-18T22:37:11Z

In the future, I'd like to see if we can't just not special-case these keys.

ghost · 2020-03-18T22:38:28Z

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.

## Summary of the Pull Request Ctrl+/ and Ctrl-? are complicated in VT input. * C-/ is supposed to be `^_` (the C0 character US) * C-? is supposed to be `DEL` * C-M-/ is supposed to be `^[^_` (ESC US) * C-M-? is supposed to be `^[^?` (ESC DEL) The astute reader will note that these characters also share the same key on a standard en-us keyboard layout, which makes the problem even more complicated. This PR does a better job of handling these weird cases. # References * #3079 - At first, I thought this PR would close this issue, but as I've learned below, this won't solve that one. This bug was merely found during that investigation. ## PR Checklist * [x] Related to #3079 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * ran tests * checked `showkey -a` in gnome-terminal, which gives you the wrong results for C-M-/, C-M-? * checked `showkey -a` in xterm, which gives you the _right_ results for C-M-/, C-M-? * checked `showkey -a` in conhost * checked `showkey -a` in Windows Terminal (cherry picked from commit d50409b)

lhecker · 2020-03-19T21:54:29Z

src/terminal/adapter/ut_adapter/inputTest.cpp

+    uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED;
+    vkey = LOBYTE(VkKeyScan(L'/'));
+    s_pwszInputExpected = L"\x1b\x1f";
+    TestKey(pInput, LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/');


BTW This particular call fails the test if you've got a non-US keyboard attached. 🤔

Yeah, that bit is unfortunate. We've got a couple other tests that make assumptions about the environment they're running in (mostly around language and system codepage) ☹️

ghost · 2020-03-20T15:18:49Z

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

Handy links:

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 labels Mar 16, 2020

carlos-zamora approved these changes Mar 16, 2020

View reviewed changes

src/terminal/input/terminalInput.cpp Outdated Show resolved Hide resolved

miniksa approved these changes Mar 17, 2020

View reviewed changes

Update src/terminal/input/terminalInput.cpp

6b22fa4

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

DHowett-MSFT reviewed Mar 17, 2020

View reviewed changes

DHowett-MSFT suggested changes Mar 17, 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 Mar 17, 2020

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

zadjii-msft mentioned this pull request Mar 18, 2020

Powershell: Ctrl-Alt-? does not work in Windows Terminal #3079

Closed

zadjii-msft added 2 commits March 18, 2020 11:06

Fix sequences in response to @j4james's comment

015e2f4

Merge branch 'dev/migrie/b/3079-C-M-questionmark' of https://github.c…

f032ccf

…om/Microsoft/Terminal into dev/migrie/b/3079-C-M-questionmark

DHowett-MSFT approved these changes Mar 18, 2020

View reviewed changes

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

ghost merged commit d50409b into master Mar 18, 2020

ghost deleted the dev/migrie/b/3079-C-M-questionmark branch March 18, 2020 22:38

lhecker reviewed Mar 19, 2020

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VT sequences for Ctrl+Alt+? input #4947

Fix VT sequences for Ctrl+Alt+? input #4947

zadjii-msft commented Mar 16, 2020 •

edited

Loading

miniksa left a comment

DHowett-MSFT Mar 17, 2020

DHowett-MSFT Mar 17, 2020

DHowett-MSFT Mar 17, 2020

j4james commented Mar 18, 2020

zadjii-msft commented Mar 18, 2020

zadjii-msft commented Mar 18, 2020

DHowett-MSFT commented Mar 18, 2020

ghost commented Mar 18, 2020

lhecker Mar 19, 2020 •

edited

Loading

DHowett-MSFT Mar 19, 2020

ghost commented Mar 20, 2020

Fix VT sequences for Ctrl+Alt+? input #4947

Fix VT sequences for Ctrl+Alt+? input #4947

Conversation

zadjii-msft commented Mar 16, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT Mar 17, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 17, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 17, 2020

Choose a reason for hiding this comment

j4james commented Mar 18, 2020

zadjii-msft commented Mar 18, 2020

zadjii-msft commented Mar 18, 2020

DHowett-MSFT commented Mar 18, 2020

ghost commented Mar 18, 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.

lhecker Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 19, 2020

Choose a reason for hiding this comment

ghost commented Mar 20, 2020

zadjii-msft commented Mar 16, 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.

lhecker Mar 19, 2020 •

edited

Loading