Spec for unified keybindings and commands, and synthesized action names #6532

zadjii-msft · 2020-06-16T15:32:17Z

Summary of the Pull Request

This is another iteration on the Command Palette spec, from #5674. These were some ideas that were tossed around by @DHowett, @cinnamon-msft and myself, formalized here. I proposed this as an addendum to the original spec, since I think the first made sense atomically, and this only makes sense as a set of changes to the original. I didn't want to go hacking up the original doc to add this set of changes.

There are two proposals in this spec - they should be viewed as two atomic units. They can be accepted or rejected independently. I'm suggesting we approve both. They work together. I'm realizing now that this is worded confusingly, and it's on me to fix that.

PR Checklist

Another spec in the Feature Request: Command Palette #2046 / Scenario: The Command Palette #5400 saga
I work here
is a doc

Abstract

This document is intended to serve as an addition to the [Command Palette Spec].
While that spec is complete in it's own right, subsequent discussion revealed
additional ways to improve the functionality and usability of the command
palette. This document builds largely on the topics already introduced in the
original spec, so readers should first familiarize themselves with that
document.

One point of note from the original document was that the original specification
was entirely too verbose when defining both keybindings and commands for
actions. Consider, for instance, a user that wants to bind the action "duplicate
the current pane". In that spec, they need to add both a keybinding and a
command:
{
    "keybindings": [
        { "keys": [ "ctrl+alt+t" ], "command": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" } },
    ],
    "commands": [
        { "name": "Duplicate Pane", "action": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" }, "icon": null },
    ]
}
These two entries are practically the same, except for two key differentiators:

the keybinding has a keys property, indicating which key chord activates the
action.

The command has a name property, indicating what name to display for the
command in the Command Palette.

What if the user didn't have to duplicate this action? What if the user could
just add this action once, in their keybindings or commands, and have it
work both as a keybinding AND a command?

zadjii-msft · 2020-06-16T15:33:57Z

I'm also going to be trying out the whole "assign everyone to the PR" thing I mentioned in chat on this PR. When you review the PR, remove yourself from "Assignees". When I address your feedback, I'll re-add you. If we hate it, then we don't need to do it again.

cinnamon-msft · 2020-06-16T21:21:23Z

I think overall thoughts for me, I prefer proposal 2. I'd rather the commands array be as minimal as possible so we don't duplicate code that already exists in keybindings. Additionally, I would argue that index doesn't need to make an appearance in the command palette and that just the profile name should be sufficient.

zadjii-msft · 2020-06-16T21:45:50Z

@cinnamon-msft Oh, this isn't an either-or situation. These are two related proposals, but they both work together. We could accept/reject them independently if we wanted

carlos-zamora

Proposal 1 is intuitive. I like the idea of just rebranding keybindings as just "commands", and everything under it is just new ways to invoke these commands.

For Proposal 2, I am concerned about localization. I know you call it out, but still :/.

I think we need both Proposal 1 and 2. When a user adds a new entry to "commands", it'd be nice if we auto-generated a name for them. I see the following scenarios:

I don't want to add this action to the command palette --> set "name" to null
I only want to add this action to the command palette --> set "keys" to null
- optionally, set "name"

I've been staring at this all day. I know I still have concerns but I think I'm at a good point to stop.

Also, sorry I repeated myself a bit in my review below!