Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
4 commits merged into from
Mar 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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'/');
Copy link
Member

@lhecker lhecker Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ☹️

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