Add support for arbitrary args in keybindings

[zadjii-msft]

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.

[DHowett-MSFT]

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]

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]

@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]

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]

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]

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

[zadjii-msft]

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.

---

[miniksa]

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.

[zadjii-msft]

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

[miniksa]

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.

[ghost]

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

Handy links:

• Release Notes
• Store Download