From 313c329bed03176b00c0be18872a6f7c8d687f00 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 16 Mar 2020 16:17:49 -0500 Subject: [PATCH 1/3] Fix VT sequences for Ctrl+Alt+? input ## 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/adapter/ut_adapter/inputTest.cpp | 28 +++++++++ src/terminal/input/terminalInput.cpp | 63 +++++++++++++++++-- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/terminal/adapter/ut_adapter/inputTest.cpp b/src/terminal/adapter/ut_adapter/inputTest.cpp index a8ae12d4681..13667f12d8a 100644 --- a/src/terminal/adapter/ut_adapter/inputTest.cpp +++ b/src/terminal/adapter/ut_adapter/inputTest.cpp @@ -684,4 +684,32 @@ void InputTest::DifferentModifiersTest() TestKey(pInput, uiKeystate, vkey, L'/'); uiKeystate = RIGHT_ALT_PRESSED; TestKey(pInput, uiKeystate, vkey, L'/'); + + // C-? -> DEL -> 0x7f + Log::Comment(NoThrowString().Format(L"Checking C-?")); + // Use SHIFT_PRESSED to force us into differentiating between '/' and '?' + vkey = LOBYTE(VkKeyScan(L'?')); + s_pwszInputExpected = L"\x7f"; + TestKey(pInput, SHIFT_PRESSED | LEFT_CTRL_PRESSED, vkey, L'?'); + TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED, vkey, L'?'); + + // C-M-/ -> M-/ -> 0x1b/ + Log::Comment(NoThrowString().Format(L"Checking C-M-/")); + uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED; + vkey = LOBYTE(VkKeyScan(L'/')); + s_pwszInputExpected = L"\x1b/"; + TestKey(pInput, LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/'); + TestKey(pInput, RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/'); + // LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr + TestKey(pInput, RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'/'); + + // C-M-? -> M-? -> 0x1b? + Log::Comment(NoThrowString().Format(L"Checking C-M-?")); + uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED; + vkey = LOBYTE(VkKeyScan(L'?')); + s_pwszInputExpected = L"\x1b?"; + TestKey(pInput, SHIFT_PRESSED | LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?'); + TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?'); + // LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr + TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'?'); } diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp index 7965a1a78fe..b33151921b1 100644 --- a/src/terminal/input/terminalInput.cpp +++ b/src/terminal/input/terminalInput.cpp @@ -198,6 +198,9 @@ static constexpr std::array s_simpleModifiedKeyMapping{ }; const wchar_t* const CTRL_SLASH_SEQUENCE = L"\x1f"; +const wchar_t* const CTRL_QUESTIONMARK_SEQUENCE = L"\x7F"; +const wchar_t* const CTRL_ALT_SLASH_SEQUENCE = L"\x1b/"; +const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b?"; void TerminalInput::ChangeKeypadMode(const bool applicationMode) noexcept { @@ -323,13 +326,61 @@ static bool _searchWithModifier(const KeyEvent& keyEvent, InputSender sender) } else { - // One last check: C-/ is supposed to be C-_ - // But '/' is not the same VKEY on all keyboards. So we have to - // figure out the vkey at runtime. - const BYTE slashVkey = LOBYTE(VkKeyScan(L'/')); - if (keyEvent.GetVirtualKeyCode() == slashVkey && keyEvent.IsCtrlPressed()) + // One last check: + // * 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 ^[? + // + // But this whole scenario is tricky. '/' is not the same VKEY on + // all keyboards. On USASCII keyboards, '/' and '?' share the _same_ + // key. So we have to figure out the vkey at runtime, and we have to + // determine if the key that was pressed was '?' with some + // modifiers, or '/' with some modifiers. + // + // These translations are not in s_simpleModifiedKeyMapping, because + // the aformentioned fact that they aren't the same VKEY on all + // keyboards. + // + // See GH#3079 for details. + + // VkKeyScan will give us both the Vkey of the key needed for this + // character, and the modifiers the user might need to press to get + // this character. + const auto slashKeyScan = VkKeyScan(L'/'); // On USASCII: 0x00bf + const auto questionMarkKeyScan = VkKeyScan(L'?'); //On USASCII: 0x01bf + + const auto ctrl = keyEvent.IsCtrlPressed(); + const auto alt = keyEvent.IsAltPressed(); + const bool shift = keyEvent.IsShiftPressed(); + + // From the KeyEvent we're translating, synthesize the equivalent VkKeyScan result + const short keyScanFromEvent = keyEvent.GetVirtualKeyCode() | + (shift ? 0x100 : 0) | + (ctrl ? 0x200 : 0) | + (alt ? 0x400 : 0); + + // If the key pressed was exactly the ? key, then try to send the + // appropriate sequence for a modified '?'. Otherwise, check if this + // was a modified '/' keypress. These mappings don't need to be + // changed at all. + if ((ctrl && alt) && WI_AreAllFlagsSet(keyScanFromEvent, questionMarkKeyScan)) + { + sender(CTRL_ALT_QUESTIONMARK_SEQUENCE); + success = true; + } + else if (ctrl && WI_AreAllFlagsSet(keyScanFromEvent, questionMarkKeyScan)) + { + sender(CTRL_QUESTIONMARK_SEQUENCE); + success = true; + } + else if ((ctrl && alt) && WI_AreAllFlagsSet(keyScanFromEvent, slashKeyScan)) + { + sender(CTRL_ALT_SLASH_SEQUENCE); + success = true; + } + else if (ctrl && WI_AreAllFlagsSet(keyScanFromEvent, slashKeyScan)) { - // This mapping doesn't need to be changed at all. sender(CTRL_SLASH_SEQUENCE); success = true; } From 6b22fa452eab594b6bfb639a6de2e8699342547d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 17 Mar 2020 11:08:04 -0500 Subject: [PATCH 2/3] Update src/terminal/input/terminalInput.cpp Co-Authored-By: Carlos Zamora --- src/terminal/input/terminalInput.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp index b33151921b1..63a78bdea2c 100644 --- a/src/terminal/input/terminalInput.cpp +++ b/src/terminal/input/terminalInput.cpp @@ -327,7 +327,7 @@ static bool _searchWithModifier(const KeyEvent& keyEvent, InputSender sender) else { // One last check: - // * C-/ is supposed to be ^_ (the C0 charater US) + // * C-/ is supposed to be ^_ (the C0 character US) // * C-? is supposed to be DEL // * C-M-/ is supposed to be ^[^/ // * C-M-? is supposed to be ^[? From 015e2f44ee905ec6540c42233e71c7c8c789e0fe Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 Mar 2020 11:06:43 -0500 Subject: [PATCH 3/3] Fix sequences in response to @j4james's comment --- src/terminal/adapter/ut_adapter/inputTest.cpp | 9 +++--- src/terminal/input/terminalInput.cpp | 30 +++++++++++++------ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/terminal/adapter/ut_adapter/inputTest.cpp b/src/terminal/adapter/ut_adapter/inputTest.cpp index 13667f12d8a..08e08267b80 100644 --- a/src/terminal/adapter/ut_adapter/inputTest.cpp +++ b/src/terminal/adapter/ut_adapter/inputTest.cpp @@ -685,6 +685,7 @@ void InputTest::DifferentModifiersTest() uiKeystate = RIGHT_ALT_PRESSED; TestKey(pInput, uiKeystate, vkey, L'/'); + // See https://github.com/microsoft/terminal/pull/4947#issuecomment-600382856 // C-? -> DEL -> 0x7f Log::Comment(NoThrowString().Format(L"Checking C-?")); // Use SHIFT_PRESSED to force us into differentiating between '/' and '?' @@ -693,21 +694,21 @@ void InputTest::DifferentModifiersTest() TestKey(pInput, SHIFT_PRESSED | LEFT_CTRL_PRESSED, vkey, L'?'); TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED, vkey, L'?'); - // C-M-/ -> M-/ -> 0x1b/ + // C-M-/ -> 0x1b0x1f Log::Comment(NoThrowString().Format(L"Checking C-M-/")); uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED; vkey = LOBYTE(VkKeyScan(L'/')); - s_pwszInputExpected = L"\x1b/"; + s_pwszInputExpected = L"\x1b\x1f"; TestKey(pInput, LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/'); TestKey(pInput, RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/'); // LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr TestKey(pInput, RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'/'); - // C-M-? -> M-? -> 0x1b? + // C-M-? -> 0x1b0x7f Log::Comment(NoThrowString().Format(L"Checking C-M-?")); uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED; vkey = LOBYTE(VkKeyScan(L'?')); - s_pwszInputExpected = L"\x1b?"; + s_pwszInputExpected = L"\x1b\x7f"; TestKey(pInput, SHIFT_PRESSED | LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?'); TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?'); // LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp index b33151921b1..f504642e460 100644 --- a/src/terminal/input/terminalInput.cpp +++ b/src/terminal/input/terminalInput.cpp @@ -199,8 +199,8 @@ static constexpr std::array s_simpleModifiedKeyMapping{ const wchar_t* const CTRL_SLASH_SEQUENCE = L"\x1f"; const wchar_t* const CTRL_QUESTIONMARK_SEQUENCE = L"\x7F"; -const wchar_t* const CTRL_ALT_SLASH_SEQUENCE = L"\x1b/"; -const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b?"; +const wchar_t* const CTRL_ALT_SLASH_SEQUENCE = L"\x1b\x1f"; +const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b\x7F"; void TerminalInput::ChangeKeypadMode(const bool applicationMode) noexcept { @@ -329,8 +329,8 @@ static bool _searchWithModifier(const KeyEvent& keyEvent, InputSender sender) // One last check: // * 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 ^[? + // * C-M-/ is supposed to be ^[^_ + // * C-M-? is supposed to be ^[^? // // But this whole scenario is tricky. '/' is not the same VKEY on // all keyboards. On USASCII keyboards, '/' and '?' share the _same_ @@ -343,6 +343,7 @@ static bool _searchWithModifier(const KeyEvent& keyEvent, InputSender sender) // keyboards. // // See GH#3079 for details. + // Also see https://github.com/microsoft/terminal/pull/4947#issuecomment-600382856 // VkKeyScan will give us both the Vkey of the key needed for this // character, and the modifiers the user might need to press to get @@ -350,36 +351,47 @@ static bool _searchWithModifier(const KeyEvent& keyEvent, InputSender sender) const auto slashKeyScan = VkKeyScan(L'/'); // On USASCII: 0x00bf const auto questionMarkKeyScan = VkKeyScan(L'?'); //On USASCII: 0x01bf + const auto slashVkey = LOBYTE(slashKeyScan); + const auto questionMarkVkey = LOBYTE(questionMarkKeyScan); + const auto ctrl = keyEvent.IsCtrlPressed(); const auto alt = keyEvent.IsAltPressed(); const bool shift = keyEvent.IsShiftPressed(); // From the KeyEvent we're translating, synthesize the equivalent VkKeyScan result - const short keyScanFromEvent = keyEvent.GetVirtualKeyCode() | + const auto vkey = keyEvent.GetVirtualKeyCode(); + const short keyScanFromEvent = vkey | (shift ? 0x100 : 0) | (ctrl ? 0x200 : 0) | (alt ? 0x400 : 0); + // Make sure the VKEY is an _exact_ match, and that the modifier + // bits also match. This handles the hypothetical case we get a + // keyscan back that's ctrl+alt+some_random_VK, and some_random_VK + // has bits that are a superset of the bits set for question mark. + const bool wasQuestionMark = vkey == questionMarkVkey && WI_AreAllFlagsSet(keyScanFromEvent, questionMarkKeyScan); + const bool wasSlash = vkey == slashVkey && WI_AreAllFlagsSet(keyScanFromEvent, slashKeyScan); + // If the key pressed was exactly the ? key, then try to send the // appropriate sequence for a modified '?'. Otherwise, check if this // was a modified '/' keypress. These mappings don't need to be // changed at all. - if ((ctrl && alt) && WI_AreAllFlagsSet(keyScanFromEvent, questionMarkKeyScan)) + if ((ctrl && alt) && wasQuestionMark) { sender(CTRL_ALT_QUESTIONMARK_SEQUENCE); success = true; } - else if (ctrl && WI_AreAllFlagsSet(keyScanFromEvent, questionMarkKeyScan)) + else if (ctrl && wasQuestionMark) { sender(CTRL_QUESTIONMARK_SEQUENCE); success = true; } - else if ((ctrl && alt) && WI_AreAllFlagsSet(keyScanFromEvent, slashKeyScan)) + else if ((ctrl && alt) && wasSlash) { sender(CTRL_ALT_SLASH_SEQUENCE); success = true; } - else if (ctrl && WI_AreAllFlagsSet(keyScanFromEvent, slashKeyScan)) + else if (ctrl && wasSlash) { sender(CTRL_SLASH_SEQUENCE); success = true;