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.

Sign up for GitHub

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

Jump to bottom

Fix Ctrl+Alt not being treated as a substitute for AltGr anymore #5552

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Fix Ctrl+Alt not being treated as a substitute for AltGr anymore #5552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/terminal/adapter/ut_adapter/inputTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ void InputTest::TerminalInputModifierKeyTests()
// Alt+Key generates [0x1b, Ctrl+key] into the stream
// Pressing the control key causes all bits but the 5 least
// significant ones to be zeroed out (when using ASCII).
if (AltPressed(uiKeystate) && ControlPressed(uiKeystate) && ch >= 0x40 && ch < 0x7F)
if (AltPressed(uiKeystate) && ControlPressed(uiKeystate) && ch > 0x40 && ch <= 0x5A)
{
s_expectedInput.clear();
s_expectedInput.push_back(L'\x1b');
Expand All @@ -491,7 +491,7 @@ void InputTest::TerminalInputModifierKeyTests()
}

// Alt+Key generates [0x1b, key] into the stream
if (AltPressed(uiKeystate) && ch != 0)
if (AltPressed(uiKeystate) && !ControlPressed(uiKeystate) && ch != 0)
{
s_expectedInput.clear();
s_expectedInput.push_back(L'\x1b');
Expand Down
20 changes: 10 additions & 10 deletions src/terminal/input/terminalInput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,16 +498,16 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
if (ch == UNICODE_NULL)
{
// For Alt+Ctrl+Key messages GetCharData() returns 0.
// The values of the ASCII characters and virtual key codes
// of <Space>, A-Z (as used below) are numerically identical.
// -> Get the char from the virtual key.
ch = LOWORD(MapVirtualKeyW(keyEvent.GetVirtualKeyCode(), MAPVK_VK_TO_CHAR));
ch = keyEvent.GetVirtualKeyCode();
Copy link
Member Author

Choose a reason for hiding this comment

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

I've taken the liberty to remove this call to MapVirtualKeyW.
As far as I now understood virtual key codes in Windows, the numeric values of the vkeys for Space and A-Z are identical with their respective ASCII value. As such a call to MapVirtualKeyW is IMO unnecessary.
Am I seing this correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you are correct.

}
if (ch == UNICODE_SPACE)
{
// Ctrl+@ and Ctrl+Space are supposed to send null bytes.
// -> Change Ctrl+Space to Ctrl+@ for compatibility reasons.
ch = 0x40;
}
if (ch >= 0x40 && ch < 0x7F)
// Alt+Ctrl acts as a substitute for AltGr on Windows.
// For instance using a German keyboard both AltGr+< and Alt+Ctrl+< produce a | (pipe) character.
// The below condition primitively ensures that we allow all common Alt+Ctrl combinations
// while preserving most of the functionality of Alt+Ctrl as a substitute for AltGr.
if (ch == UNICODE_SPACE || (ch > 0x40 && ch <= 0x5A))
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see: this change is safe because 0x20 & 0b11111 is 0 (we didn't need to set it to 0x40 just to strip off the top few bits). Clever.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DHowett-MSFT Did you ever see this article? https://garbagecollected.org/2017/01/31/four-column-ascii/
You probably know everything about this already, but for me it was pretty eye opening regardless...
It quickly shows you why ^␣ is actually the same as ^@ and why ^[ is the same as the ESC character. It's a much better representation of ASCII as the square one you can find everywhere else (including Wikipedia). 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I actually used to have a giant version of this table printed in my office, which has been immensely helpful

{
// Pressing the control key causes all bits but the 5 least
// significant ones to be zeroed out (when using ASCII).
Expand All @@ -530,7 +530,7 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)

// This section is similar to the Alt modifier section above,
// but handles cases without Ctrl modifiers.
if (keyEvent.IsAltPressed() && keyEvent.GetCharData() != 0)
if (keyEvent.IsAltPressed() && !keyEvent.IsCtrlPressed() && keyEvent.GetCharData() != 0)
Copy link
Member Author

@lhecker lhecker Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've had to add these two additional checks, because Alt+Ctrl combinations actually sometimes do carry character data!
For instance Ctrl+Alt+< will send an KeyEvent with the CharData being |. 😲
The old condition here (keyEvent.IsAltPressed() && keyEvent.GetCharData() != 0) would thus incorrectly match and Ctrl+Alt+< would send ^[| instead of |.

(This wasn't an issue in the past when HandleKey was only called with CharData in a much more limited set of circumstances. Nowadays it's almost always invoked with CharData.)

{
_SendEscapedInputSequence(keyEvent.GetCharData());
return true;
Expand All @@ -544,7 +544,7 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
// -> Send a "null input sequence" in that case.
// We don't need to handle other kinds of Ctrl combinations,
// as we rely on the caller to pretranslate those to characters for us.
if (keyEvent.IsCtrlPressed())
if (!keyEvent.IsAltPressed() && keyEvent.IsCtrlPressed())
{
const auto ch = keyEvent.GetCharData();
const auto vkey = keyEvent.GetVirtualKeyCode();
Expand Down