Skip to content

Commit

Permalink
Fix VT sequences for Ctrl+Alt+? input (#4947)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
zadjii-msft authored and DHowett committed Mar 19, 2020
1 parent 266402a commit 7deaf6b
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 6 deletions.
29 changes: 29 additions & 0 deletions src/terminal/adapter/ut_adapter/inputTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -684,4 +684,33 @@ void InputTest::DifferentModifiersTest()
TestKey(pInput, uiKeystate, vkey, L'/');
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 '?'
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-/ -> 0x1b0x1f
Log::Comment(NoThrowString().Format(L"Checking C-M-/"));
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'/');
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-? -> 0x1b0x7f
Log::Comment(NoThrowString().Format(L"Checking C-M-?"));
uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED;
vkey = LOBYTE(VkKeyScan(L'?'));
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
TestKey(pInput, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'?');
}
75 changes: 69 additions & 6 deletions src/terminal/input/terminalInput.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ static constexpr std::array<TermKeyMap, 6> 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\x1f";
const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b\x7F";

void TerminalInput::ChangeKeypadMode(const bool applicationMode) noexcept
{
Expand Down Expand Up @@ -323,13 +326,73 @@ 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 character 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.
// 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
// this character.
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 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) && wasQuestionMark)
{
sender(CTRL_ALT_QUESTIONMARK_SEQUENCE);
success = true;
}
else if (ctrl && wasQuestionMark)
{
sender(CTRL_QUESTIONMARK_SEQUENCE);
success = true;
}
else if ((ctrl && alt) && wasSlash)
{
sender(CTRL_ALT_SLASH_SEQUENCE);
success = true;
}
else if (ctrl && wasSlash)
{
// This mapping doesn't need to be changed at all.
sender(CTRL_SLASH_SEQUENCE);
success = true;
}
Expand Down

0 comments on commit 7deaf6b

Please sign in to comment.