Implement Alt-Numpad handling #17637
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
DHowett
reviewed
Jul 31, 2024
Comment on lines
-1466
to
-1509
| if (vkey == VK_MENU && !down) | ||
| { | ||
| // Manually generate an Alt KeyUp event into the key bindings or terminal. | ||
| // This is required as part of GH#6421. | ||
| (void)_TrySendKeyEvent(VK_MENU, scanCode, modifiers, false); | ||
| handled = true; | ||
| } | ||
| else if ((vkey == VK_F7 || vkey == VK_SPACE) && down) | ||
| { | ||
| // Manually generate an F7 event into the key bindings or terminal. | ||
| // This is required as part of GH#638. | ||
| // Or do so for alt+space; only send to terminal when explicitly unbound | ||
| // That is part of #GH7125 | ||
| auto bindings{ _core.Settings().KeyBindings() }; | ||
| auto isUnbound = false; | ||
| const KeyChord kc = { | ||
| modifiers.IsCtrlPressed(), | ||
| modifiers.IsAltPressed(), | ||
| modifiers.IsShiftPressed(), | ||
| modifiers.IsWinPressed(), | ||
| gsl::narrow_cast<WORD>(vkey), | ||
| 0 | ||
| }; | ||
|
|
||
| if (bindings) | ||
| { | ||
| handled = bindings.TryKeyChord(kc); | ||
|
|
||
| if (!handled) | ||
| { | ||
| isUnbound = bindings.IsKeyChordExplicitlyUnbound(kc); | ||
| } | ||
| } | ||
|
|
||
| const auto sendToTerminal = vkey == VK_F7 || (vkey == VK_SPACE && isUnbound); | ||
|
|
||
| if (!handled && sendToTerminal) | ||
| { | ||
| // _TrySendKeyEvent pretends it didn't handle F7 for some unknown reason. | ||
| (void)_TrySendKeyEvent(gsl::narrow_cast<WORD>(vkey), scanCode, modifiers, true); | ||
| // GH#6438: Note that we're _not_ sending the key up here - that'll | ||
| // get passed through XAML to our KeyUp handler normally. | ||
| handled = true; | ||
| } |
There was a problem hiding this comment.
I'm assuming these just got subsumed into the key handler, and none of this special logic to ensure the following keys work is needed?
- release alt -> send key to app (win32IM)
- F7/Space pressed -> activate key bindings, or send to terminal
There was a problem hiding this comment.
From a glance it looked like all of that code is covered by our generic key handling already.
Alt key up? That's a fall-through to _TrySendKeyEvent anyway already. ✅
Alt+Space? We do a keybinding lookup, it fails, it falls through to _TrySendKeyEvent. ✅
F7? We do a keybinding lookup, it fails, it falls through to _TrySendKeyEvent. ✅
Seems good to me. In my testing it was.
DHowett
reviewed
Jul 31, 2024
| codepage = ansi ? 1252 : 437; | ||
| } | ||
|
|
||
| // The OS code seemed to also simply cut off the last byte in the accumulator. |
There was a problem hiding this comment.
I was 🤏 this close to just scrapping the entire weird ACP/OEM support. It's not supported in any application that uses CP_UTF8 as its ACP anyway, so I'm not sure if there's any point for us to support it either.
DHowett
approved these changes
Aug 2, 2024
carlos-zamora
approved these changes
Aug 6, 2024
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
microsoft-github-policy-service
bot
added
Issue-Bug
DHowett
pushed a commit
that referenced
this pull request
Aug 23, 2024
This adds an indirection for `_KeyHandler` so that `OnDirectKeyEvent` can call `_KeyHandler`. This allows us to consistently handle Alt-key-up events. Then I added custom handling for Alt+ddd (OEM), Alt+0ddd (ANSI), and Alt+'+'+xxxx (Unicode) sequences, due to the absence of Alt-key events with xaml islands and our TSF control. Closes #17327 ## Validation Steps Performed * Tested it according to https://conemu.github.io/en/AltNumpad.html * Unbind Alt+Space * Run `showkey -a` * Alt+Space generates `^[ ` * F7 generates `^[[18~` (cherry picked from commit 2fab986) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSCpCg Service-Version: 1.21
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds an indirection for
_KeyHandlerso thatOnDirectKeyEventcan call
_KeyHandler. This allows us to consistently handleAlt-key-up events. Then I added custom handling for Alt+ddd (OEM),
Alt+0ddd (ANSI), and Alt+'+'+xxxx (Unicode) sequences, due to the
absence of Alt-key events with xaml islands and our TSF control.
Closes #17327
Validation Steps Performed
showkey -a^[^[[18~