Skip to content

Commit

Permalink
spec: update the spec for Action IDs (#16816)
Browse files Browse the repository at this point in the history
As we start to work on implementing Action IDs, the spec written a few
years ago needs some updates. This PR makes those updates for the
current implementation plan.

References #6899
  • Loading branch information
PankajBhojwani authored Mar 9, 2024
1 parent 274eaae commit 7f02b25
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 25 deletions.
69 changes: 44 additions & 25 deletions doc/specs/#6899 - Action IDs/#6899 - Action IDs.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ contexts without needing to replicate an entire json blob.

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

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

The goal is to introduce an `id` parameter by which actions could be uniquely
referred to. If we'd ever like to use an action outside the list of `actions`, we
Expand All @@ -36,38 +36,54 @@ 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
We will then restructure `defaults.json`, and also users' settings files (via `fixUpUserSettings`), in the following manner:
* Instead of each `command` json block containing both the `action` (along with additional arguments) and the `keys`, these will now be split up -
* There will now be one json block for just the `command`/`action`, which will also contain the `id`. These json blocks will be in their own list called `actions`.
* There will be another json block for the `keys`, which will refer to the action to be invoked by `id`. These json blocks will be in their own list called `keybindings`.

For example, let's take a look at the `split pane right` action in `defaults.json` as we currently have it:

`"actions": [..., { "command": { "action": "splitPane", "split": "right" }, "keys": "alt+shift+plus" }, ...]`

This will now become:

`"actions": [..., { "command": { "action": "splitPane", "split": "right" }, "id": "Terminal.SplitPaneRight" }, ...]`

`"keybindings": [..., { "keys": "alt+shift+plus", "id": "Terminal.SplitPaneRight" }, ...]`

Here is how we will parse settings file and construct the relevant settings model objects:
* We will first scan the `actions` list. 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.
global `id->ActionAndArgs` map. All actions defined in `defaults.json` must have an `id` specified, and all actions provided by fragments must also have `id`s. Any actions from the defaults or fragments that do not provide `id`s will be ignored. As for user-specified commands, if no `id` is set, we will auto-generate one for that command based on the action and any additional arguments. For example, the `split pane right` command above might result in an autogenerated id `User.SplitPaneRight`.
* Note: this step is also where we will generate _commands_. We will use the name provided in the entry if it's set or the action's `GenerateName` method.
* Next we will scan the `keybindings` list. These entries will
create a `KeyChord->ActionAndArgs` entry in the keybindings map. Since these objects should all contain an `id`, we will simply use the `id->ActionAndArgs` map we created in the previous step. Any object with `keys` set but no `id` will be ignored.

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

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

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

![figure 3](data-mockup-actions-and-ids.png)
We allow certain actions that take a form like this:

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`:
```
{
// Select color scheme...
"name": { "key": "SetColorSchemeParentCommandName" },
"commands": [
{
"iterateOn": "schemes",
"name": "${scheme.name}",
"command": { "action": "setColorScheme", "colorScheme": "${scheme.name}" }
}
]
}
```

![figure 4](data-mockup-actions-and-ids-and-keys.png)
For this case, having an `id` on the top level could potentially make sense when it comes to using that `id` in a menu, but not in the case of using that `id` for a keybinding. For the initial implementation, we will not support an `id` for these types of actions, which leaves us open to revisiting this in the future.

### Layering

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
Expand All @@ -87,6 +103,9 @@ 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.

As for fragments, all actions in fragments _must_ have an `id`. If a fragment provides an action without an `id`, or provides an `id` that clashes with one of the actions in `defaults.json`, that action will be ignored.

> 👉 NOTE: This will mean that actions will now need an `OriginTag`, similar to profiles and color schemes
### Existing Scenarios

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 7f02b25

Please sign in to comment.