Fix Ctrl+Alt not being treated as a substitute for AltGr anymore#5552
Fix Ctrl+Alt not being treated as a substitute for AltGr anymore#5552DHowett-MSFT merged 1 commit intomicrosoft:masterfrom lhecker:fix-5525-altgr-alt-ctrl
Conversation
This issue was caused by a9c9714.
| // 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(); |
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.
| // 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
left a comment
There was a problem hiding this comment.
Okay I'm thinking that with the research you've got here, this might actually be more robust than it used to be. Thanks for linking directly to what this originally looked like, made reviewing this much easier.
DHowett-MSFT
left a comment
There was a problem hiding this comment.
This seems reasonable to me. Thank you 😄
| // 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: |
|
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...|,~,{,[,],}