Support remapping keybindings by zadjii-msft · Pull Request #748 · microsoft/terminal

zadjii-msft · 2019-05-13T20:07:24Z

Fixes #537.

Adds support for remappable keybindings in the profiles.json file. The keybindings look like the following:

    "keybindings": [
        {
            "keys": ["ctrl+t"],
            "command": "newTab"
        },
        {
            "keys": ["ctrl+shift+1"],
            "command": "newTabProfile0"
        },
        { "keys": ["ctrl+w"], "command": "closeTab" },
        { "keys": ["ctrl+tab"], "command": "nextTab" },
        { "keys": ["ctrl+shift+tab"], "command": "prevTab" },
        { "keys": ["ctrl+shift+pgup"], "command": "scrollUp" },
    ]

…t reloaded

Instead iterate over the action names, then serialize in that order, so it's sane.

This fixes #537

src/cascadia/TerminalApp/AppKeyBindings.cpp

src/cascadia/TerminalApp/AppKeyBindings.idl

src/cascadia/TerminalSettings/KeyChord.cpp

src/cascadia/TerminalApp/AppKeyBindings.cpp

src/cascadia/TerminalSettings/KeyChord.cpp

DHowett-MSFT · 2019-05-14T01:11:58Z

is now a good time to split the "key bindings" model from the key bindings magic? I'm concerned that the KeyBindings/AppKeyBindings object has a little too much power.

Right now, it's responsible for:

loading key bindings
saving them
creating the defaults
processing key input on request
dispatching the event associated with a key binding

Should those last two live somewhere else?

mdtauk · 2019-05-14T01:15:07Z

Everytime a new keybinding is added to the app, an entry for a customised keybinding should be added to the docs and to the profile.json file (commented out until the user wishes to change it from the default)

This way all keybindings are discoverable.

zadjii-msft · 2019-05-14T13:57:23Z

@DHowett-MSFT I'm not sure if I totally get what you're saying - seems to me like you're proposing a separate KeyBindingsSerialization class?

I'm not really strongly opposed to that, but I'm not really sure what extra benefit we'd get from that. To me it seems like the serialization of the bindings is pretty tightly coupled to the actual implementation of having a map of keychord->action pairs.

Also, iirc bullet point 3 is actually handled by CascadiaSettings::_CreateDefaultKeybindings

carlos-zamora · 2019-05-14T21:50:16Z

src/cascadia/TerminalSettings/KeyChord.cpp

+            {
+                modifiers |= KeyModifiers::Alt;
+            }
+            else if (part == SHIFT_KEY)


Do we have to worry about left-shift vs right-shift? Do we want to?

We recently learned that right alt is altgr sometimes?

This is true, but I'm not sure UWP can differentiate between the two in the KeyEvent. I'm pretty sure there's only a Ctrl, Shift, and Alt, and not a left/right version :/

If we find out otherwise, then the entire KeyModifiers structure might need to be updated, as well as any consumers.

carlos-zamora · 2019-05-14T22:16:38Z

src/cascadia/TerminalSettings/KeyChord.cpp

+                // If we weren't able to find a match, throw an exception.
+                if (!foundKey)
+                {
+                    throw hresult_invalid_argument();


Do we also want an error dialogue box like in #622? Or maybe just ignore it? I'm ok with this behavior for now but we should probably decide on behavior and file it as a separate issue for after.

Yea probably, but I'll make that a separate task for once both this and 622 are merged.

Here's a concern: should not being able to deserialize a single key binding blast the entire profiles into outer space? even with an error message, that sucks

Yea, I don't think it should. I'd think that failing to parse a single key would still ignore the bad binding, but display the error (somehow)

src/cascadia/TerminalSettings/KeyChord.cpp

…dings # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp

* Update the appkeybindings vector to a map * Update the strings to wchar_t*'s to avoid ctoring a ton on startup

src/cascadia/TerminalApp/AppKeyBindings.cpp

src/cascadia/TerminalApp/AppKeyBindings.idl

src/cascadia/TerminalSettings/KeyChord.cpp

DHowett-MSFT · 2019-05-15T20:43:50Z

src/cascadia/TerminalSettings/KeyChord.cpp

+                // If we weren't able to find a match, throw an exception.
+                if (!foundKey)
+                {
+                    throw hresult_invalid_argument();


Here's a concern: should not being able to deserialize a single key binding blast the entire profiles into outer space? even with an error message, that sucks

src/cascadia/TerminalApp/AppKeyBindings.cpp

DHowett-MSFT · 2019-05-15T20:58:48Z

More to the point about idl, my concern is that key binding serialization lives in a totally different component than settings serialization now.. hmm. Is this something we can reconcile?

zadjii-msft · 2019-05-15T21:04:24Z

@DHowett-MSFT great point on that. The TerminalApp is the only thing doing any serializing, it should probably be the one responsible for doing this serializing. I could add a helper class for serializing just the keychord, like KeyChordSerializer, with just a couple statics on it.

The AppKeyBindings is actually an App component, so it makes sense to couple it's serialization directly to it's class, just like profiles, globals and schemes.

* flip the names, actions map around

DHowett-MSFT

This seems mostly reasonable to me.

src/cascadia/TerminalApp/AppKeyBindings.cpp

DHowett-MSFT

Though, we changed + to plus, but - . , are still symbolic. someone in the future might bind ctrl+- and ctrl+plus as zoom out and zoom in respectively, and that seems unsymmetric

timheuer · 2019-05-16T22:40:07Z

Was looking at this in conjunction with resolving suggestions for #805. The XAML framework uses Windows::System::VirtualKey and Windows::System::VirtualKeyModifiers...is there a reason that Terminal should create their own types here that are now maintaining modifiers and enum values for something that already exists? Retrieving the keybindings with a KeyChord that has it's own values is proving to be challenging using them anywhere in the UI due to different types and not being able to cast them accordingly well.

zadjii-msft · 2019-05-17T13:14:35Z

@timheuer that's a really good idea. I've pulled it out to #877, because yea, there's really no need to use our own types.

src/cascadia/TerminalApp/AppKeyBindings.cpp

src/cascadia/TerminalApp/KeyChordSerialization.cpp

…dings # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp

zadjii-msft · 2019-05-20T20:40:31Z

@DHowett-MSFT is CLA bot acting up? Can we manually trigger it somehow? Looks like it's broken on #710 too

zadjii-msft added 5 commits May 10, 2019 14:03

Add support for serializing keybindings

ed657a0

This is close, but at some point we lose the keybindings when they ge…

d3b9fa1

…t reloaded

Fix actually loading the keys

82f4fff

Don't serialize over the unordered_map

0e3a197

Instead iterate over the action names, then serialize in that order, so it's sane.

Clean up for PR

66592b5

This fixes #537

zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-Settings Issues related to settings and customizability, for console or terminal labels May 13, 2019

zadjii-msft added this to the Windows Terminal v1.0 milestone May 13, 2019

zadjii-msft requested review from a team, DHowett-MSFT, adiviness, carlos-zamora and miniksa May 13, 2019 20:07

DHowett-MSFT reviewed May 13, 2019

View reviewed changes

mKay00 reviewed May 13, 2019

View reviewed changes

src/cascadia/TerminalApp/AppKeyBindings.cpp Outdated Show resolved Hide resolved

mKay00 reviewed May 13, 2019

View reviewed changes

src/cascadia/TerminalApp/AppKeyBindings.cpp Outdated Show resolved Hide resolved

adiviness suggested changes May 13, 2019

View reviewed changes

mKay00 mentioned this pull request May 13, 2019

Adding keymapping for access to Settings #684

Merged

mKay00 mentioned this pull request May 14, 2019

Move the solution to VS2019 and v142 toolset #565

Closed

carlos-zamora reviewed May 14, 2019

View reviewed changes

src/cascadia/TerminalSettings/KeyChord.cpp Outdated Show resolved Hide resolved

carlos-zamora reviewed May 14, 2019

View reviewed changes

src/cascadia/TerminalSettings/KeyChord.cpp Outdated Show resolved Hide resolved

zadjii-msft added 3 commits May 15, 2019 14:16

Merge remote-tracking branch 'origin/master' into dev/migrie/f/keybin…

23e7859

…dings # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp

Some PR nits

8727085

well that was easier than expected

4b75c85

* Update the appkeybindings vector to a map * Update the strings to wchar_t*'s to avoid ctoring a ton on startup

DHowett-MSFT suggested changes May 15, 2019

View reviewed changes

DHowett-MSFT reviewed May 15, 2019

View reviewed changes

src/cascadia/TerminalApp/AppKeyBindings.cpp Show resolved Hide resolved

zadjii-msft added 2 commits May 15, 2019 17:16

* use a constexpr wstring_view

3eaabfa

* flip the names, actions map around

Move the serialization of keychords into a seperate file.

7aee31a

DHowett-MSFT approved these changes May 15, 2019

View reviewed changes

src/cascadia/TerminalApp/AppKeyBindings.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/AppKeyBindings.cpp Show resolved Hide resolved

DHowett-MSFT reviewed May 15, 2019

View reviewed changes

zadjii-msft mentioned this pull request May 16, 2019

Modifying profiles.json causes multiple settings reloads #858

Closed

bitcrazed modified the milestones: Windows Terminal v1.0, Terminal v0.2 May 16, 2019

zadjii-msft mentioned this pull request May 17, 2019

KeyChord should just use the existing WinRT types instead of defining our own #877

Closed

adiviness approved these changes May 20, 2019

View reviewed changes

src/cascadia/TerminalApp/AppKeyBindings.cpp Show resolved Hide resolved

src/cascadia/TerminalApp/AppKeyBindings.cpp Show resolved Hide resolved

src/cascadia/TerminalApp/KeyChordSerialization.cpp Outdated Show resolved Hide resolved

zadjii-msft mentioned this pull request May 20, 2019

Add an error dialog when we fail to parse settings #903

Merged

4 tasks

zadjii-msft added 2 commits May 20, 2019 14:28

Some minor PR nits

f13d343

Merge remote-tracking branch 'origin/master' into dev/migrie/f/keybin…

6b7c16b

…dings # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp

zadjii-msft merged commit 29e3808 into master May 21, 2019

zadjii-msft deleted the dev/migrie/f/keybindings branch May 21, 2019 14:26

timheuer mentioned this pull request May 21, 2019

Adding auto-UI shortcuts to menu based on keymappings #924

Merged

5 tasks

Conversation

zadjii-msft commented May 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT commented May 14, 2019

Uh oh!

mdtauk commented May 14, 2019

Uh oh!

zadjii-msft commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett-MSFT commented May 15, 2019

Uh oh!

zadjii-msft commented May 15, 2019

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

timheuer commented May 16, 2019

Uh oh!

zadjii-msft commented May 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft commented May 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants