Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec for global action IDs #6902

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,14 @@ And assuming the user has bound:
context of the current tab or control in the json. Think for example about
"Close tab" or "Close other tabs" - currently, those work by _knowing_ which
tab the "action" is specifed for, not by actually using a `closeTab` action.
In the future, they might need to be implemented as something like
- Close Tab: `{ "action": "closeTab", "index": "${selectedTab.index}" }`
- Close Other Tabs: `{ "action": "closeTabs", "otherThan": "${selectedTab.index}" }`
- Close Tabs to the Right: `{ "action": "closeTabs", "after": "${selectedTab.index}" }`
* In the future, they might need to be implemented as something like
- Close Tab: `{ "action": "closeTab", "index": "${selectedTab.index}" }`
- Close Other Tabs: `{ "action": "closeTabs", "otherThan": "${selectedTab.index}" }`
- Close Tabs to the Right: `{ "action": "closeTabs", "after":
"${selectedTab.index}" }`
* We could alternatively just imply that when those actions are specified in
the tab context menu, they're implicitly defined as being for the current
tab. That might be simpler.


<!-- Footnotes -->
Expand Down
228 changes: 228 additions & 0 deletions doc/specs/#6899 - Action IDs/#6899 - Action IDs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
---
author: Mike Griese @zadjii-msft
created on: 2020-07-13
last updated: 2020-07-22
issue id: 6899
---

# Action IDs

## Abstract

This document is intended to serve as an addition to the [Command Palette Spec],
as well as the [New Tab Menu Customization Spec].

As we come to rely more on actions being a mechanism by which the user defines
"do something in the Terminal", we'll want to make it even easier for users to
re-use the actions that they've already defined, as to reduce duplicated json as
much as possible. This spec proposes a mechanism by which actions could be
uniquely identifiable, so that the user could refer to bindings in other
contexts without needing to replicate an entire json blob.

## Solution Design

This spec was largely inspired by the following diagram from @DHowett:

![figure 1](data-mockup.png)

The goal is to introduce an `id` parameter by which actions could be uniquely
refered to. If we'd ever like to use an action outside the list of `actions`, we
can simply refer to the action's ID, allowing the user to only define the action
_once_.

We'll start by renaming `bindings` to `actions`. `bindings` was suggested as a
rename for `keybindings` in [#6532], as a way to make the name more generic.
Discussion with the team lead to the understanding that the name `actions` would
be even better, as a way of making the meaning of the "list of actions" more
obvious.

When we're parsing `actions`, we'll make three passes:
* The first pass will scan the list for objects with an `id` property. We'll
attempt to parse those entries into `ActionAndArgs` which we'll store in the
global `id->ActionAndArgs` map. If any entry doesn't have an `id` set, we'll
skip it in this phase. If an entry doesn't have a `command` set, we'll ignore
it in this pass.
* The second pass will scan for _keybindings_. Any entries with `keys` set will
create a `KeyChord->ActionAndArgs` entry in the keybindings map. If the entry
has an `id` set, then we'll simply re-use the action we've already parsed for
the `id`, from the action map. If there isn't an `id`, then we'll parse the
action manually at this time. Entries without a `keys` set will be ignored in
this pass.
* The final pass will be to generate _commands_. Similar to the keybindings
pass, we'll attempt to lookup actions for entries with an `id` set. If there
isn't an `id`, then we'll parse the action manually at this time. We'll then
get the name for the entry, either from the `name` property if it's set, or
the action's `GenerateName` method.

For a visual representation, let's assume the user has the following in their
`actions`:

![figure 2](data-mockup-actions.png)

We'll first parse the `actions` to generate the mapping of `id`->`Actions`:

![figure 3](data-mockup-actions-and-ids.png)

Then, we'll parse the `actions` to generate the mapping of keys to actions, with
some actions already being defined in the map of `id`->`Actions`:

![figure 4](data-mockup-actions-and-ids-and-keys.png)


When layering `actions`, if a later settings file contains an action with the
same `id`, it will replace the current value. In this way, users can redefine
actions, or remove default ones (with something like `{ "id":
"Terminal.OpenTab", "command":null }`

We'd maintain a large list of default actions, each with unique `id`s set. These
are all given `id`'s with a `Terminal.` prefix, to easily identify them as
built-in, default actions. Not all of these actions will be given keys, but they
will all be given `id`s.

> 👉 NOTE: The IDs for the default actions will need to be manually created, not
> autogenerated. These `id`s are not strings displayed in the user interface, so
> localization is not a concern.

As we add additional menus to the Terminal, like the customization for the new
tab dropdown, or the tab context menu, or the `TermControl` context menu, they
could all refer to these actions by `id`, rather than duplicating the same json.


### Existing Scenarios

Keybindings will still be stored as a `keys->Action` mapping, so the user will
still be able to override default keybindings exactly the same as before.

Similarly, commands in the Command Palette will continue using their existing
`name->Action` mapping they're currently using. For a binding like

```json
{ "keys": "ctrl+alt+x", "id": "Terminal.OpenDefaultSettings" },
```
* We'll bind whatever action is defined as `Terminal.OpenDefaultSettings` to
<kbd>ctrl+alt+x</kbd>.
* We'll use whatever action is defined as `Terminal.OpenDefaultSettings` to
generate a name for the command palette.

### Future Context Menus
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

In [New Tab Menu Customization Spec], we discuss allowing the user to bind
actions to the new tab menu. In that spec, they can do so with something like
the following:

```json
{
"newTabMenu": [
{ "type":"action", "command": { "action": "adjustFontSize", "delta": 1 }, }
{ "type":"action", "command": { "action": "adjustFontSize", "delta": -1 }, }
{ "type":"action", "command": "resetFontSize", }
{ "type":"profile", "profile": "cmd" },
{ "type":"profile", "profile": "Windows PowerShell" },
{ "type":"separator" },
{
"type":"folder",
"name": "Settings...",
"icon": "C:\\path\\to\\icon.png",
"entries":[
{ "type":"action", "command": "openSettings" },
{ "type":"action", "command": { "action": "openSettings", "target": "defaultsFile" } },
]
}
]
}
```

In this example, the user has also exposed the "Increase font size", "Decrease
font size", and "Reset font size" actions, as well as the settings files in a
submenu. With this proposal, the above could instead be re-written as:

```json
{
"newTabMenu": [
{ "type":"action", "id": "Terminal.IncreaseFontSize" },
{ "type":"action", "id": "Terminal.DecreaseFontSize" },
{ "type":"action", "id": "Terminal.ResetFontSize" },
{ "type":"profile", "profile": "cmd" },
{ "type":"profile", "profile": "Windows PowerShell" },
{ "type":"separator" },
{
"type":"folder",
"name": "Settings...",
"icon": "C:\\path\\to\\icon.png",
"entries":[
{ "type":"action", "id": "Terminal.OpenDefaultSettings" },
{ "type":"action", "id": "Terminal.OpenSettings" },
]
}
]
}
```
Comment on lines +139 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I love this. It looks a lot cleaner.


In this example, the actions are looked up from the global map using the `id`
provided, enabling the user to re-use their existing definitions. If the user
re-defined the `Terminal.IncreaseFontSize` action to mean something else, then
the action in the new tab menu will also be automatically updated.

Furthermore, when additional menus are added (such as the tab context menu, or
the `TermControl` context menu), these could also leverage a similar syntax to
the above to allow re-use of the `id` parameter.

Discussion with the team also suggested that users shouldn't be able to define
actions in these menus _at all_. The actions should exclusively be defined in
`actions`, and other menus should only be able to refer to these actions by
`id`.

## UI/UX Design

There's not a whole lot of UI for this feature specifically. This is largely
behind-the-scenes refactoring of how actions can be defined.

## Capabilities

### Accessibility

_(not applicable)_

### Security

_(no change expected)_

### Reliability

_(no change expected)_

### Compatibility

_(no change expected)_

### Performance, Power, and Efficiency

_(no change expected)_

## Potential Issues

This won't necessarily play well with iterable commands in the Command Palette,
but that's okay. For iterable commands, users will still need to define the
actions manually.

## Future considerations

* See the following issues for other places where this might be useful:
- [#1912] - Context Menu for Tabs
* See also [#5524], [#5025], [#5633]
- [#3337] - Right-click menu inside TerminalControl (w/ Copy & Paste?)
* See also [#5633] and [#5025], both those actions seem reasonable in either
the tab context menu or the control context menu.

<!-- Footnotes -->
[Command Palette Spec]: https://github.com/microsoft/terminal/blob/master/doc/specs/%232046%20-%20Command%20Palette.md
[New Tab Menu Customization Spec]: https://github.com/microsoft/terminal/blob/master/doc/specs/%231571%20-%20New%20Tab%20Menu%20Customization.md

[#1571]: https://github.com/microsoft/terminal/issues/1571
[#1912]: https://github.com/microsoft/terminal/issues/1912
[#3337]: https://github.com/microsoft/terminal/issues/3337
[#5025]: https://github.com/microsoft/terminal/issues/5025
[#5524]: https://github.com/microsoft/terminal/issues/5524
[#5633]: https://github.com/microsoft/terminal/issues/5633
[#6532]: https://github.com/microsoft/terminal/issues/6532
[#6899]: https://github.com/microsoft/terminal/issues/6899
Binary file added doc/specs/#6899 - Action IDs/data-mockup-002.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added doc/specs/#6899 - Action IDs/data-mockup.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.