Add support for arbitrary args in keybindings #3391

zadjii-msft · 2019-10-31T14:54:14Z

Summary of the Pull Request

Enables the user to provide arbitrary argument values to shortcut actions through a new args member of keybindings. For some keybindings, like NewTabWithProfile<N>, we previously needed 9 different ShortcutActions, one for each value of Index. If a user wanted to have a NewTabWithProfile11 keybinding, that was simply impossible. Now that the args are in their own separate json object, each binding can accept any number of arbitrary argument values.

So instead of:

        { "command": "newTab", "keys": ["ctrl+shift+t"] },
        { "command": "newTabProfile0", "keys": ["ctrl+shift+1"] },
        { "command": "newTabProfile1", "keys": ["ctrl+shift+2"] },
        { "command": "newTabProfile2", "keys": ["ctrl+shift+3"] },
        { "command": "newTabProfile3", "keys": ["ctrl+shift+4"] },

We can now use:

        { "command": "newTab", "keys": ["ctrl+shift+t"] },
        { "command": { "action": "newTab", "index": 0 }, "keys": ["ctrl+shift+1"] },
        { "command": { "action": "newTab", "index": 1 }, "keys": ["ctrl+shift+2"] },
        { "command": { "action": "newTab", "index": 2 }, "keys": ["ctrl+shift+3"] },

Initially, this does seem more verbose. However, for cases where there are multiple args, or there's a large range of values for the args, this will quickly become a more powerful system of expressing keybindings.

The "legacy" keybindings are left in in this PR. They have helper methods to generate appropriate IActionArgs values. Prior to releasing 1.0, I think we should remove them, if only to remove some code bloat.

References

See the spec for more details.

This is part two of the implementation, part one was #2446

PR Checklist

Closes Feature Request: Support arbitrary arguments for keybindings #1142
I work here
Tests added/passed
Schema updated

Validation Steps Performed

Ran Tests
Removed the legacy keybindings from the defaults.json, everything still works
Tried leaving the legacy keybingings in my profiles.json, everything still works.

…inding(ShortcutAction, KeyChord) implementation out, which I don't totally understand

DHowett-MSFT · 2019-10-31T21:46:28Z

I hate to suggest this, but.. is args part of command? Should it be? if it is, we could double-parse the command object for the name and the args:

"command": {
    "name": "newTab",
    "index": 4
}

command.name = look up actionargs type, actargs.FromJson(commandObject)?

DHowett-MSFT · 2019-10-31T21:47:02Z

That might make the model a little easier to understand when we expand it to "command palette", binding menu items to "commands", etc. The command has all the data necessary to act

zadjii-msft · 2019-10-31T22:13:25Z

@DHowett-MSFT another idea: we just put them right in the root directly.

            { "command": "switchToTab", "keys": [ "alt+shift+1" ], "index": 2 },
            { "command": "splitPane", "keys": [ "alt+shift+|" ], "type": "vertical", "profileGuid": "{etc}" },

DHowett-MSFT · 2019-10-31T22:21:14Z

Interesting! Will we ever have a command that has a keys?

Like:

{ "command": "sendKeys", "keys": ["alt+left", "ctrl+q"], "keys": ["shift-1"] }

Obviously, activated with Shift+1 and when you activate it it prints Alt+Left,Ctrl+Q into the input buffer. 😁

zadjii-msft · 2019-11-01T13:08:53Z

Frick. That's a good point. Lets loop back on the previous proposal then:

    "keybindings":
    [
        { "command": "scrollUpPage", "keys": ["ctrl+shift+pgup"] },
        { "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] },
        { "command": { "action": "switchToTab", "index": 1 }, "keys": ["ctrl+alt+2"] },
        { "command": { "action": "sendInput", "keys": ["alt+left", "ctrl+q"] }, "keys": ["shift+1"] }
    ]

If "command" is a string, we'll assume it doesn't have args, and we'll try and parse it as the action.

Otherwise, we'll try and use the "action" property to get the ShortcutAction.

I'm proposing "action" here, because for #2046, commands will need another separate name property that we can use for displaying:

    "commands":
    [
        { 
            "name": "Scroll up a page", 
            "icon":null,
            "command": "scrollUpPage" 
        },
        { 
            "name": "Switch to tab 0", 
            "icon":null,
            "command": { "action": "switchToTab", "index": 0 } 
        },
        { 
            "name": "Switch to the second tab", 
            "icon":null,
            "command": { "action": "switchToTab", "index": 1 } 
        },
        { 
            "name": "Do a send input", 
            "icon":null,
            "command": { "action": "sendInput", "keys": ["alt+left", "ctrl+q"] }
         }
    ]

Oh no. Now I hate myself. Why do commands and keybindings need to be in separate arrays at all? (I'm putting this on in an expando because it's getting long)

    "commands":
    [
        { 
            "name": "Scroll up a page", 
            "icon":null,
            "command": "scrollUpPage",
            "keys": ["ctrl+shift+pgup"]
        },
        { 
            "name": "Switch to tab 0", 
            "icon":null,
            "command": { "action": "switchToTab", "index": 0 },
            "keys": ["ctrl+alt+1"]
        },
        { 
            "name": "Switch to the second tab", 
            "icon":null,
            "command": { "action": "switchToTab", "index": 1 },
            "keys": ["ctrl+alt+2"]
        },
        { 
            "name": "Do a send input", 
            "icon":null,
            "command": { "action": "sendInput", "keys": ["alt+left", "ctrl+q"] },
            "keys": ["shift+1"]
         },
        { 
            // I'm just a keybinding, not a command
            "command": { "action": "sendInput", "keys": ["alt+right", "ctrl+w"] },
            "keys": ["shift+2"]
        },
        { 
            "name": "I'm just a command, unbound", 
            "icon":null,
            "command": { "action": "sendInput", "keys": ["ctrl+left", "ctrl+1"] }
        },
        { 
            "name": { "key": "SomeLocalizedXAMLStringResource"}, 
            "icon":null,
            "command": { "action": "switchToTab", "index": 15 },
            "keys": ["ctrl+alt+5"]
        }
    ]

We'd just have all commands and bindings in a singular array.

DHowett-MSFT · 2019-11-03T01:17:55Z

I would love to have this followed up with a PR that adds profileGuid to newTab 😁

zadjii-msft · 2019-11-05T17:46:10Z

Looping back on this - Dustin and I discussed, and the combined commands/keybindings situation is a madhouse, which is part of the reason we're leaving it for 2.0.

However, there was goodness we agreed on, which is the following:

    "keybindings":
    [
        { "command": "scrollUpPage", "keys": ["ctrl+shift+pgup"] },
        { "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] },
        { "command": { "action": "switchToTab", "index": 1 }, "keys": ["ctrl+alt+2"] },
        { "command": { "action": "sendInput", "keys": ["alt+left", "ctrl+q"] }, "keys": ["shift+1"] }
    ]

If "command" is a string, we'll assume it doesn't have args, and we'll try and parse it as the action.

Otherwise, we'll try and use the "action" property to get the ShortcutAction.

…rbitrary-args # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.idl # src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

@DHowett-MSFT

This is a change to make @DHowett-MSFT happy. Changes the args to be a part of the "command" object, as opposed to an object on their own. EX: ```jsonc // Old style { "command": "switchToTab0", "keys": ["ctrl+1"] }, { "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] }, // new style { "command": "switchToTab0", "keys": ["ctrl+1"] }, { "command": "switchToTab", "args": { "index": 0 } "keys": ["ctrl+alt+1"] }, ```

…rbitrary-args

miniksa · 2019-11-06T17:43:24Z

The "legacy" keybindings are left in in this PR. They have helper methods to generate appropriate IActionArgs values. Prior to releasing 1.0, I think we should remove them, if only to remove some code bloat.

TODO without work item ID attached.

src/cascadia/TerminalApp/ActionArgs.h

src/cascadia/TerminalApp/defaults.json

zadjii-msft · 2019-11-08T14:22:42Z

@zadjii-msft Make sure to update the spec before you submit this PR

* Add a `Direction::None` * LOAD BEARING * add some GH ids to TODOs

miniksa · 2019-11-13T22:23:49Z

src/cascadia/TerminalApp/ActionArgs.h

+    };
+
+    // Possible Direction values
+    static constexpr std::string_view LeftString{ "left" };


Shouldn't constant string patterns like this for parsing go wherever the rest of the parsing is for things like dimensions?

miniksa

Per verbal discussion, link todo to make a direction parser later so rando strings aren't floating in a rando file.

carlos-zamora

Just some comments not worth blocking over.

src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

src/cascadia/TerminalApp/AppKeyBindings.h

src/cascadia/TerminalApp/ActionAndArgs.cpp

## Summary of the Pull Request With #3391, I almost certainly regressed the ability for the new tab dropdown to display the keybindings for each profile. This adds them back. ## PR Checklist * [x] Closes #3603 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Now, we can lookup keybindings not only for `ShortcutAction`s, but also `ActionAndArgs`s, so we can look up the binding for an action with a particular set of arguments. --------------------------------------------- * fixes #3603 by searching for ActionAndArgs too * Apply suggestions from code review Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

ghost · 2019-11-26T17:27:57Z

🎉Windows Terminal Preview v0.7.3291.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 9 commits October 29, 2019 17:14

this is a start, but there's a weird linker bug if I take the SetKeyb…

2bb524c

…inding(ShortcutAction, KeyChord) implementation out, which I don't totally understand

a good old-fashioned clean will fix that right up

8799451

all these things work

d93a0bc

hey this actually _functionally_ works

e9a809b

Mostly cleanup and completion of implementation

26d9035

Hey I bet we could just make NewTab the handler for NewTabWithProfile

4e43186

Start writing tests for Keybinding args

9f426a7

Add tests

03a6486

Revert a bad sln change, and clean out dead code

7ce59ba

zadjii-msft mentioned this pull request Nov 5, 2019

Add a spec for the Command Palette #2193

Merged

5 tasks

zadjii-msft added 8 commits November 5, 2019 11:57

Merge branch 'master' into dev/migrie/f/1142-arbitrary-args

14f942b

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

64a4a4f

…rbitrary-args # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.idl # src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

schemas are hard yo

adf9698

Fix the build?

8623279

wonder why my -Wall settings are different than CI...

6cb6eba

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

01b8c8f

…rbitrary-args

this makes me hate things

fb0c987

miniksa requested changes Nov 6, 2019

View reviewed changes

src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 6, 2019

zadjii-msft mentioned this pull request Nov 7, 2019

Automatically generate ShortcutAction code from ~JSON Schema~ X-Macros #3475

Closed

15 tasks

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 8, 2019

This was referenced Nov 11, 2019

Display a warning if a keybinding didn't have a required arg #3522

Closed

Remove deserialization of legacy json keys #1069

Closed

zadjii-msft added 2 commits November 11, 2019 10:33

Comments from PR

d47db70

* Add a `Direction::None` * LOAD BEARING * add some GH ids to TODOs

Merge branch 'master' into dev/migrie/f/1142-arbitrary-args

0c67298

zadjii-msft requested a review from miniksa November 11, 2019 21:59

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Nov 11, 2019

zadjii-msft requested review from carlos-zamora and DHowett-MSFT November 11, 2019 21:59

zadjii-msft mentioned this pull request Nov 12, 2019

Keyboard Selection Spec #2840

Merged

miniksa reviewed Nov 13, 2019

View reviewed changes

miniksa approved these changes Nov 13, 2019

View reviewed changes

zadjii-msft requested a review from leonMSFT November 14, 2019 00:13

add a comment

fed692c

carlos-zamora approved these changes Nov 14, 2019

View reviewed changes

PR nits from carlos

d78560a

zadjii-msft merged commit 6a4c737 into master Nov 14, 2019

This was referenced Nov 14, 2019

Open panes with a specific profile #998

Closed

Add keybinding: moveTabLeft, moveTabRight #3593

Closed

DHowett-MSFT mentioned this pull request Nov 16, 2019

Dropdown menu no longer displays bound keys (after #3391) #3603

Closed

zadjii-msft mentioned this pull request Nov 18, 2019

Re-add keybindings to new tab dropdown #3618

Merged

3 tasks

ghost mentioned this pull request Nov 26, 2019

Feature Request: Support arbitrary arguments for keybindings #1142

Closed

zadjii-msft deleted the dev/migrie/f/1142-arbitrary-args branch January 31, 2020 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for arbitrary args in keybindings #3391

Add support for arbitrary args in keybindings #3391

zadjii-msft commented Oct 31, 2019 •

edited

Loading

DHowett-MSFT commented Oct 31, 2019

DHowett-MSFT commented Oct 31, 2019

zadjii-msft commented Oct 31, 2019

DHowett-MSFT commented Oct 31, 2019

zadjii-msft commented Nov 1, 2019

DHowett-MSFT commented Nov 3, 2019

zadjii-msft commented Nov 5, 2019

miniksa commented Nov 6, 2019

zadjii-msft commented Nov 8, 2019

miniksa Nov 13, 2019

miniksa left a comment

carlos-zamora left a comment

ghost commented Nov 26, 2019

Add support for arbitrary args in keybindings #3391

Add support for arbitrary args in keybindings #3391

Conversation

zadjii-msft commented Oct 31, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

DHowett-MSFT commented Oct 31, 2019

DHowett-MSFT commented Oct 31, 2019

zadjii-msft commented Oct 31, 2019

DHowett-MSFT commented Oct 31, 2019

zadjii-msft commented Nov 1, 2019

DHowett-MSFT commented Nov 3, 2019

zadjii-msft commented Nov 5, 2019

miniksa commented Nov 6, 2019

zadjii-msft commented Nov 8, 2019

miniksa Nov 13, 2019

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Nov 26, 2019

zadjii-msft commented Oct 31, 2019 •

edited

Loading