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 2 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
28 changes: 28 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,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'/');
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-? -> 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'?');
}
63 changes: 57 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/";
const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b?";

void TerminalInput::ChangeKeypadMode(const bool applicationMode) noexcept
{
Expand Down Expand Up @@ -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 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.

// 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() |
Copy link
Contributor

Choose a reason for hiding this comment

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

this is quite clever!

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

(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))
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
{
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;
}
Expand Down