Allow the mapping of OEM keys ({}|\<>/_-=+) in key bindings #2067
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
This commit introduces support for key bindings containing keys traditionally classified as "OEM" keys. It uses VkKeyScanW and MapVirtualKeyW, and translates the modifiers that come out of VkKeyScanW to key chord modifiers. The net result of this is that you can use bindings like "ctrl+|" in your settings. That one in particular will be reserialized (and displayed in any menus) as "ctrl+shift+\". Admittedly, this is not clear, but it _is_ the truest representation of the key. This commit also moves the Xaml key chord name override generator into App as a static function, *AND* it forces its use for all modifier names. This will present a localization issue, which will be helped in part by #1972. This is required to work around microsoft/microsoft-ui-xaml#708. I've kept the original code around guarded by a puzzling ifdef, because it absolutely has value. Fixes #1212.
|
I reckon @shanselman will like this. Also, it makes |
zadjii-msft
approved these changes
Jul 23, 2019
| // Method Description: | ||
| // - Takes a MenuFlyoutItem and a corresponding KeyChord value and creates the accelerator for UI display. | ||
| // Takes into account a special case for an error condition for a comma | ||
| // Arguments: | ||
| // - MenuFlyoutItem that will be displayed, and a KeyChord to map an accelerator | ||
| void App::_SetAcceleratorForMenuItem(Windows::UI::Xaml::Controls::MenuFlyoutItem& menuItem, const winrt::Microsoft::Terminal::Settings::KeyChord& keyChord) | ||
| { | ||
| #ifdef DEP_MICROSOFT_UI_XAML_708_FIXED |
There was a problem hiding this comment.
Is this right?
Until 708 is fixed, always use the KeyboardAcceleratorTextOverride method.
When 708 is fixed, if it's not a comma, set the Keyboard accelerator. If it is a comma, still use the override thing.
Would we still need the else when 708 is fixed?
There was a problem hiding this comment.
We'd probably yank the comma check as well, and the other accelerator formatting code. I wanted this diff to be "approximately" minimal, however
|
Magical.
…On Tue, Jul 23, 2019 at 6:15 AM Mike Griese ***@***.***> wrote:
***@***.**** approved this pull request.
God bless you for fixing this one. I think I'm a little confused by the
whole DEP_MICROSOFT_UI_XAML_708_FIXED thing - maybe coffee hasn't kicked
in quite yet. I'm otherwise fine with this.
------------------------------
In src/cascadia/TerminalApp/App.cpp
<#2067 (comment)>:
> // Method Description:
// - Takes a MenuFlyoutItem and a corresponding KeyChord value and creates the accelerator for UI display.
// Takes into account a special case for an error condition for a comma
// Arguments:
// - MenuFlyoutItem that will be displayed, and a KeyChord to map an accelerator
void App::_SetAcceleratorForMenuItem(Windows::UI::Xaml::Controls::MenuFlyoutItem& menuItem, const winrt::Microsoft::Terminal::Settings::KeyChord& keyChord)
{
+#ifdef DEP_MICROSOFT_UI_XAML_708_FIXED
Is this right?
Until 708 is fixed, always use the KeyboardAcceleratorTextOverride method.
When 708 is fixed, if it's not a comma, set the Keyboard accelerator. If
it is a comma, still use the override thing.
Would we still need the else when 708 is fixed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2067?email_source=notifications&email_token=AAAAWTHCHWSNHIOQAC3ZT2DQA375RA5CNFSM4IF76VM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7IZ7KI#pullrequestreview-265396137>,
or mute the thread
<https:/notifications/unsubscribe-auth/AAAAWTAGDD2VXIWKEKBYI6LQA375RANCNFSM4IF76VMQ>
.
|
miniksa
approved these changes
Jul 23, 2019
DHowett-MSFT
commented
Jul 23, 2019
|
🎉 Handy links: |
ghost
mentioned this pull request
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 commit introduces support for key bindings containing keys
traditionally classified as "OEM" keys. It uses VkKeyScanW and
MapVirtualKeyW, and translates the modifiers that come out of
VkKeyScanW to key chord modifiers.
The net result of this is that you can use bindings like "ctrl+|" in
your settings. That one in particular will be reserialized (and
displayed in any menus) as "ctrl+shift+". Admittedly, this is not
clear, but it is the truest representation of the key.
This commit also moves the Xaml key chord name override generator into
App as a static function, AND it forces its use for all modifier
names. This will present a localization issue, which will be helped in
part by #1972. This is required to work around
microsoft/microsoft-ui-xaml#708. I've kept the original code around
guarded by a puzzling ifdef, because it absolutely has value.
Fixes #1212.
Renormalization
"alt+=""alt+plus""alt+:""alt+shift+;""ctrl+?""ctrl+shift+/""shift+~""shift+<backtick>"(markdown limitation)
"alt+}""alt+shift+]""ctrl+alt+<pipe>""ctrl+alt+shift+\\""shift+\"""shift+'""alt+,""alt+,""ctrl+-""ctrl+-""shift+.""shift+."PR Checklist
Validation Steps Performed
Manual