[Spec] Settings Model - Actions

doc/specs/#885 - Terminal Settings Model/Actions Addendum.md

@@ -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.
+  - 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`.
+ - 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.
+   - 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).
+
+### 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.
+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.
+
+
+## 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...
+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.
+
+## 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