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] Settings Model - Actions #9428

Merged
9 commits merged into from
May 5, 2021
Merged
269 changes: 269 additions & 0 deletions doc/specs/#885 - Terminal Settings Model/Actions Addendum.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
---
author: Carlos Zamora @carlos-zamora
created on: 2021-03-12
last updated: 2021-03-12
issue id: [#885]
---

# Actions in the Settings Model

## Abstract

This spec proposes a refactor of how Windows Terminal actions are stored in the settings model.
The new representation would mainly allow the serialization and deserialization of commands and keybindings.

## Inspiration

A major component that is missing from the Settings UI is the representation of keybindings and commands.
The JSON represents both of these as a combined entry as follows:
```js
{ "icon": "path/to/icon.png", "name": "Copy the selected text", "command": "copy", "keys": "ctrl+c" },
```
In the example above, the copy action is...
- bound to <kbd>ctrl+c</kbd>
- presented as "Copy the selected text" with the "path/to/icon.png" icon

However, at the time of writing this spec, the settings model represents it as...
- (key binding) a `KeyChord` to `ActionAndArgs` entry in a `KeyMapping`
- (command) a `Command` with an associated icon, name, and action (`ActionAndArgs`)

This introduces the following issues:
1. Serialization
- We have no way of knowing when a command and a key binding point to the same action. Thus, we don't
know when to write a "name" to the json.
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
- We also don't know if the name was auto-generated or set by the user. This can make the JSON much more bloated by
actions with names that would normally be autogenerated.
2. Handling Duplicates
- The same action can be bound to multiple key chords. The command palette combines all of these actions into one entry
because they have the same name. In reality, this same action is just being referenced in different ways.

## Solution Design

I propose that the issues stated above be handled via the following approach.

### Step 1: Consolidating actions

`ActionAndArgs` and `Command` will be combined to look like the following:

```c++
runtimeclass Action
{
// The path to the icon (or icon itself, if it's an emoji)
String IconPath;

// The associated name. If none is defined, one is auto-generated.
String Name;

// The key binding that can be used to invoke this action.
IReference<KeyChord> Keys;

// The action itself.
// NOTE: This can be represented as an ActionAndArgs to simplify the work that needs to be done.
ShortcutAction Command;
IActionArgs Args;

// Future Considerations:
// - [#6899]: Action IDs --> add an identifier here
// - source tracking --> add a tag here
// - action grouping (i.e. clipboard, pane management, etc...) --> expose a getter here
}
```

The goal here is to consolidate key binding actions and command palette actions into a single class.
This will also require the following supplemental changes:
- `Action::LayerJson`
- This must combine the logic of `KeyMapping::LayerJson` and `Command::LayerJson`.
- Modifying the key chord
- The logic for `KeyMapping::SetKeyBinding` and `KeyMapping::ClearKeyBinding` can be moved to `Action`.
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
- Key Chord text
- `String Action::KeyChordText{ get; }` can be exposed to pass the text directly to the command palette.
This would depend on `Keys` and, thus, propagate changes automatically.
- Observable properties
- Several members can be exposed as observable (as they are in `Command`) such as the name, key chord text, and icon.
- Nested and iterable commands
- `HasNestedCommands`, `NestedCommands{ get; }`, `IterateOn` will continue to be exposed.
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
- A setter for these customizations will not be exposed until we find it necessary (i.e. adding support for customizing it in the Settings UI)
- Command expansion can continue to be exposed here to reduce implementation cost.

Overall, the new `Action` class will be an evolution of the `Command` class that now also includes the `KeyChord` it has.
This allows the implementation cost of this step to be relatively small. In fact, the class itself may not even be renamed.

Completion of this step should only cause relatively minor changes to anything that depends on `Command`, because
it is largely the same class. However, key bindings will largely be impacted because we represent key bindings as
a map of `KeyChord`s to `ActionAndArgs`. This leads us to step 2 of this process.


### Step 2: Querying actions

Key bindings and commands are deserialized by basically storing individual actions to a map.
- `KeyMapping` is basically an `IMap<KeyChord, ActionAndArgs>` with a few extra functions. In fact, it actually
stores key binding data to a `std::map<KeyChord, ActionAndArgs>` and directly interacts with it.
- `Command::LayerJson` populates an `IMap<String, Command>` during deserialization as it iterates over every action.
Note that `Command` can be interpreted as a wrapper for `ActionAndArgs` with more stuff here.

It makes sense to store these actions as maps. So, following step 1 above, we can also store and expose actions
something like the following:

```c++
runtimeclass ActionMap
{
Action GetActionByName(String name);
Action GetActionByKeyChord(KeyChord keys);

KeyChord GetKeyBindingForAction(ShortcutAction action);
KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs);

// Future Considerations:
// - [#6899]: Action IDs --> GetActionByID()
// - Getters for groups of actions...
// - IMap<> GetActionsByGrouping
// - IMap<> GetActionsByTag
}
```

The getters will return null if a matching action or key chord is not found. Internally, we can
store the actions as follows:

```c++
std::map<std::wstring, Action> _NameMap;
std::map<KeyChord, Action> _KeyMap;
```

`GetActionByName` and `GetActionByKeyChord` will directly query the internal maps.
`GetKeyBindingForAction` will iterate through the `_KeyMap` to find a match (similar to how it is now).

Copy link
Member

Choose a reason for hiding this comment

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

Should we mention layering here too? Because the layering more applies to the container of objects than it does the individual Actions. Like, when you have

{ "keys": "ctrl+a", "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}

then we'll need to first set the KeyChordText of the copy action to "ctrl+a", but then when we go to replace copy in the keymap with paste, we'll need to clear that copy's KeyChordText when we replace it with paste

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a layering section for the defaults.json and settings.json layering.

In an event like you described above where both occur in the same layer, my plan is to keep "copy" in the action list, but actually have "paste" be bound to ctrl+a. Serialization-wise, we'll only output:

{ "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}

I think this is what our current actions model does, because we just iterate over all of our actions and add them to the command palette independently of their key chord.

### Step 3: Settings UI needs

After the former two steps are completed, the new representation of actions in the settings model is now on-par with
what we have today. In order to bind these new actions to the Settings UI, we need the following:

1. Exposing the maps
- `ActionMap::KeyBindings` and `ActionMap::Commands` will need to be added to pass the full list of actions to
the Settings UI.
- In doing this, we can already update the Settings UI to include a better view of our actions.
2. Creating a copy of the settings model
- The Settings UI operates by binding the XAML controls to a copy of the settings model.
- See "copying the action map" in the potential issues.
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
3. Adding setters to `Action`
- `ActionMap` must listen for changes to actions as they get added to the map:
- If `Name` changes, update `_NameMap`
- If `Keys` changes, update `_KeyMap`
- In the event that name/key-chord is set to something that's already taken, we need to propagate those changes to
the rest of `ActionMap`. As we do with the JSON, we respect the last name/key-chord set by the user.
4. Serialization
- `Action::ToJson()` and `ActionMap::ToJson()` should perform most of the work for us.
- See "unbinding actions" in potential issues.
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved


carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
## UI/UX Design

N/A

## Capabilities

N/A

### Accessibility

N/A

### Security

N/A

### Reliability

N/A

### Compatibility

N/A

### Performance, Power, and Efficiency

## Potential Issues

### Copying the `ActionMap`

The Settings UI needs `ActionMap::Copy()` so that it can bind XAML controls to the clone. However, the design
above has two internal maps that own their `Action`s. Unfortunately, STL does not have multi-indexed data
structures. To get around this issue, we can introduce internal action IDs to ensure we don't duplicate an action twice.

```c++
std::map<std::wstring, unsigned int> _NameMap;
std::map<KeyChord, unsigned int> _KeyMap;
std::map<unsigned int, Action> _IDMap;

unsigned int _nextID = 1;
```

With this design, the internal action ID can be saved as an `unsigned int`. This ID is never exposed.
When an `Action` is added to the `ActionMap`, we update `_IDMap` and increment `_nextID`.

Now that there are no duplicates between `_NameMap` and `_KeyMap`, creating a copy of `_NameMap` and
`_KeyMap` is trivial. We can iterate over `_IDMap` to create a copy of every `Action` stored
and save it to a new `_IDMap`.

### Unbinding actions

Removing a name or a key chord from an `Action` propagates the change to the `ActionMap`.

Removing the action itself is more complicated...
Copy link
Member

Choose a reason for hiding this comment

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

Wait could you elaborate more on how we get into the "remove an action" state? Like, removing the keys from an action, I get why we'd do that. I get why we'd set the name of an action to null. But why are we replacing an action with Unbound? Like, what's the user story here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We talked a bit about this in the spec meeting, and I've updated the spec to kinda cover this a bit better. Let me know if it's still not clear though :)

1. On the `Action`, set `ShortcutAction = Unbound` and `IActionArgs = nullptr`
2. The associated name and `KeyChord` are mapped to this "unbound" action

The act of unbinding an action like this is intended to free a key binding or remove a string
from the command palette. In explicitly storing an "unbound" action, we are explicitly
saying that this key chord must be passed through or this string must be removed.

When exposing the action to the command palette, we must ensure that the string is removed (or not added).
When exposing the action to the key binding, we must ensure that the key chord's action is returned as
unhandled.

We specifically need an "unbound" action for serialization. This way, we can output something like this
in the JSON:

```js
{ "command": "unbound", "keys": "false" }
```

In making a distinction between "unbound" actions and "invalid" actions, we can theoretically also
serialize invalid actions.

carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
## Future considerations

There are a number of ideas regarding actions that would be fairly trivial to implement given this refactor:
- [#6899]: Action IDs
- As actions grow to become more widespread within Windows Terminal (i.e. dropdown and jumplist integration),
a formal ID system would help users reference the same action throughout the app. With the internal
ID system introduced in the "copying the action map" section, we would simply introduce a new
`std:map<string, unsigned int> _ExternalIDMap` that is updated like the others, and add a `String ID`
property to `Action`.
- [#8100] Source Tracking
- Identifying where a setting came from can be very beneficial in the settings model and UI. For example,
profile settings now have an `OverrideSource` getter that describes what `Profile` object the setting
came from (i.e. base layer, profile generator, etc...). A similar system can be used for `Action` in
that we record if the action was last modified in defaults.json or settings.json.
- There seems to be no desire for action inheritance (i.e. inheriting the name/key-chord from the parent).
So this should be sufficient. In the event that this feature is desired, `ActionMap` would have to be
modified to link to a parent `ActionMap`, but the implementation details of this effort are out of
scope for this spec.
- Action Grouping
- Actions are clearly organized into different meta-groups such as window management, tab management,
pane management, etc. There would be some benefits to assigning `ShortcutAction`s to different groups
like breaking up the Settings UI's Actions page into multiple lists by category (though this could
arguably be a responsibility of the view model). Another possible benefit would be to provide context
for when an action is valid (i.e. window-level, pane-level, etc...). This would do some of the work
for [#8767], and allow for window-level actions to be accessed via the command palette and key bindings.

## Resources

[#885]: https://github.com/microsoft/terminal/issues/885
[#6899]: https://github.com/microsoft/terminal/issues/6899
[#8100]: https://github.com/microsoft/terminal/issues/8100
[#8767]: https://github.com/microsoft/terminal/issues/8767

Other references:
[Settings UI: Actions Page]: https://github.com/microsoft/terminal/issues/6900
[Settings UI: Actions Page Design]: https://github.com/microsoft/terminal/pulls/9427
[Action ID Spec]: https://github.com/microsoft/terminal/issues/7175