Fix Ctrl+Alt not being treated as a substitute for AltGr anymore #5552
Conversation
This issue was caused by a9c9714.
| // -> Get the char from the virtual key. | ||
| ch = LOWORD(MapVirtualKeyW(keyEvent.GetVirtualKeyCode(), MAPVK_VK_TO_CHAR)); | ||
| ch = keyEvent.GetVirtualKeyCode(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I believe you are correct.
| @@ -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) | |||
There was a problem hiding this comment.
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.)
|
BTW did something change recently? |
|
I hope that's it 😁 |
zadjii-msft
added
Area-Input
| // 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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). 🙂
There was a problem hiding this comment.
I actually used to have a giant version of this table printed in my office, which has been immensely helpful
|
🎉 Handy links: |
ghost
mentioned this pull request
|
I think this is a misguided change and should be reverted, see my comment at |
|
🎉 Once again, thanks for the contribution! This pull request was included in a set of conhost changes that was just |
Summary of the Pull Request
This PR fixes #5525 by re-adding range checks that where erroneously removed in a9c9714.
PR Checklist
Validation Steps Performed
<,+,7,8,9,0while holding either Alt+Ctrl or AltGr and...|,~,{,[,],}