Add enhanced key support for ConPty #5021

carlos-zamora · 2020-03-19T19:57:45Z

Summary of the Pull Request

ConPty did not set the ENHANCED_KEY flag when generating new input. This change helps detect when it's supposed to do so, and sends it.

References

Enhanced Key Documentation

PR Checklist

Closes ConPTY does not translate arrow keys with +ENHANCED_KEY in their modifiers #2397

Detailed Description of the Pull Request / Additional comments

KEY_EVENT_RECORD modifiers	VT encodable?	Detectable on the way out?
CAPSLOCK_ON	No	No
ENHANCED_KEY	No	Yes**
LEFT_ALT_PRESSED	Yes*	Yes*
LEFT_CTRL_PRESSED	Yes*	Yes*
NUMLOCK_ON	No	No
RIGHT_ALT_PRESSED	Yes*	Yes*
RIGHT_CTRL_PRESSED	Yes*	Yes*
SCROLLLOCK_ON	No	No
SHIFT_PRESSED	Yes	Yes

* We can detect Alt and Ctrl, but not necessarily which one
** Enhanced Keys are limited to the following:
    - off keypad: INS, DEL, HOME, END, PAGE UP, PAGE DOWN, direction keys
    - on keypad: / and ENTER
   Since we can't detect the keypad keys, those will _not_ send the ENHANCED_KEY modifier.
   For the following CSI action codes, we can assume that they are Enhanced Keys:
     case CsiActionCodes::ArrowUp:
     case CsiActionCodes::ArrowDown:
     case CsiActionCodes::ArrowRight:
     case CsiActionCodes::ArrowLeft:
     case CsiActionCodes::Home:
     case CsiActionCodes::End:
     case CsiActionCodes::CSI_F1:
     case CsiActionCodes::CSI_F3:
     case CsiActionCodes::CSI_F2:
     case CsiActionCodes::CSI_F4:
   These cases are handled in ActionCsiDispatch

Validation Steps Performed

Followed bug repro steps. It now matches!

carlos-zamora · 2020-03-19T19:59:18Z

Can someone verify that the table above is correct? Specifically, the "No?" cells. Because if there is a way to know that, I could go ahead and maybe add them in in this PR.

DHowett-MSFT · 2020-03-19T20:24:45Z

There's no way for us to pass lock state over VT.

DHowett-MSFT · 2020-03-19T20:25:31Z

nit: please edit your markdown table to be fully spaced. commit messages don't actually use markdown, and are presented in standard monospace type

DHowett-MSFT

This is clever and looks like it works well.

DHowett-MSFT · 2020-03-19T20:27:36Z

Make sure @zadjii-msft signs off.

Please do not automerge this pull request -- the bot will take the single commit message as the merge message and not take into account the PR title or description

DHowett-MSFT · 2020-03-19T20:27:44Z

Is this testable?

carlos-zamora · 2020-03-19T20:36:00Z

Is this testable?

I just followed the repro steps from the attached but. Recreated the python script and everything. The output now matches. :)

DHowett-MSFT · 2020-03-19T21:10:12Z

I meant is this unit-testable 😉

carlos-zamora · 2020-03-19T22:03:41Z

I meant is this unit-testable 😉

Looks like this would be best tested as a part of #4405, so I'm gonna try and fix that here too.

src/terminal/parser/InputStateMachineEngine.cpp

j4james · 2020-03-21T18:32:50Z

There's no way for us to pass lock state over VT.

I'm not sure if you were just referring to our current implementation, but technically it should be possible for us to control the NumLock and CapsLock states via VT sequences if we really wanted to. See DECNUMLK and DECCAPSLK.

DHowett-MSFT · 2020-03-21T18:49:37Z

Ah! I wasn’t aware of those. Are they intended to be generated by a terminal? I wouldn’t want to contravene best practice.

It’ll be really useful to look at these when we are planning robust key event serialization for Win32 compatibility :)

j4james · 2020-03-21T22:52:59Z

Ah! I wasn’t aware of those. Are they intended to be generated by a terminal?

Ah no. These are for the host to report the state to the client. Going in the other direction would probably require DECPCTERM mode I think, and then you're basically just registering the key being pressed rather than reporting the state. Another alternative would be the extended keyboard reports from DECKPM mode, which can give you the CapsLock state (and also the left/right Shift and Alt states), with every keypress.

zadjii-msft

Yea these are pretty much all nits

src/terminal/parser/ut_parser/InputEngineTest.cpp

src/terminal/parser/InputStateMachineEngine.cpp

ghost · 2020-03-24T19:45:34Z

Hello @carlos-zamora!

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.

nwillems94 · 2020-03-24T19:59:15Z

Thanks for the good work! When should users expect to see these changes? Are ConPty updates pushed through Windows Update?

zadjii-msft · 2020-03-24T20:07:45Z

@nwillems94 If you're using the Windows Terminal, you'll get this update on the next release of the Windows Terminal ☺️

If you're still using the vintage console, then yea, you'll need to wait for a Windows Update. Conpty is actually just conhost.exe. If you're on Insiders, these updates usually take about a month after when they're checked in here. If you're not, then I honestly have no idea how long it is till the update that will come after 2004^{(the Windows build, not the year)}.

Improve wide glyph support in UIA (GH-4946) Add enhanced key support for ConPty (GH-5021) Set DxRenderer non-text alias mode (GH-5149) Reduce CursorChanged Events for Accessibility (GH-5196) Add more object ID tracing for Accessibility (GH-5215) Add SS3 cursor key encoding to ConPty (GH-5383) UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399) Make CodepointWidthDetector::GetWidth faster (CC-3727) add til::math, use it for float conversions to point, size (GH-5150) Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353) Fix a deadlock and a bounding rects issue in UIA (GH-5385) Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398) Reimplement the VT tab stop functionality (CC-5173) Clamp parameter values to a maximum of 32767. (CC-5200) Prevent the cursor type being reset when changing the visibility (CC-5251) Make RIS switch back to the main buffer (CC-5248) Add support for the DSR-OS operating status report (CC-5300) Update the virtual bottom location if the cursor moves below it (CC-5317) ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352) Set Cascadia Code as default font (GH-5121) Show a double width cursor for double width characters (GH-5319) Delegate all character input to the character event handler (CC-4192) Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092) Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079 Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122) Render row-by-row instead of invalidating entire screen (GH-5185) Make conechokey use ReadConsoleInputW by default (GH-5148) Manually pass mouse wheel messages to TermControls (GH-5131) This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208) Fix copying wrapped lines by implementing better scrolling (GH-5181) Emit lines wrapped due to spaces at the end correctly (GH-5294) Remove unneeded whitespace (CC-5162)

ghost · 2020-04-22T17:12:27Z

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

Handy links:

conpty: add enhanced key support

b020f4b

DHowett-MSFT reviewed Mar 19, 2020

View reviewed changes

DHowett-MSFT approved these changes Mar 19, 2020

View reviewed changes

carlos-zamora requested a review from zadjii-msft March 19, 2020 20:47

add testing and polish enhanced keys

df38dc1

DHowett-MSFT suggested changes Mar 20, 2020

View reviewed changes

src/terminal/parser/InputStateMachineEngine.cpp Show resolved Hide resolved

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 20, 2020

don't allow CSI_F# to be enhanced

305ce4e

zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-Input Related to input processing (key presses, mouse, etc.) labels Mar 23, 2020

zadjii-msft approved these changes Mar 24, 2020

View reviewed changes

Griese PR Comments

0ca2c0d

DHowett-MSFT approved these changes Mar 24, 2020

View reviewed changes

fix 'possible loss of data'

dbec9b0

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 24, 2020

ghost merged commit 403069c into master Mar 24, 2020

ghost deleted the dev/cazamor/conpty/enhanced-key branch March 24, 2020 19:46

ghost mentioned this pull request Apr 22, 2020

ConPTY does not translate arrow keys with +ENHANCED_KEY in their modifiers #2397

Closed

zadjii-msft mentioned this pull request Nov 6, 2023

dwControlKeyState behaviour changed between Windows 10/Windows 11 #16266

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enhanced key support for ConPty #5021

Add enhanced key support for ConPty #5021

carlos-zamora commented Mar 19, 2020 •

edited

Loading

carlos-zamora commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT left a comment

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

carlos-zamora commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

carlos-zamora commented Mar 19, 2020

j4james commented Mar 21, 2020

DHowett-MSFT commented Mar 21, 2020

j4james commented Mar 21, 2020

zadjii-msft left a comment

ghost commented Mar 24, 2020

nwillems94 commented Mar 24, 2020 •

edited

Loading

zadjii-msft commented Mar 24, 2020

ghost commented Apr 22, 2020

Add enhanced key support for ConPty #5021

Add enhanced key support for ConPty #5021

Conversation

carlos-zamora commented Mar 19, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

carlos-zamora commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

carlos-zamora commented Mar 19, 2020

DHowett-MSFT commented Mar 19, 2020

carlos-zamora commented Mar 19, 2020

j4james commented Mar 21, 2020

DHowett-MSFT commented Mar 21, 2020

j4james commented Mar 21, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

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

nwillems94 commented Mar 24, 2020 • edited Loading

zadjii-msft commented Mar 24, 2020

ghost commented Apr 22, 2020

carlos-zamora commented Mar 19, 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.

nwillems94 commented Mar 24, 2020 •

edited

Loading