[Spec] Settings Model - Actions

.github/actions/spelling/expect/expect.txt

@@ -18,6 +18,7 @@ ACIOSS
 ACover
 actctx
 ACTCTXW
+actionmap
 activatable
 ACTIVEBORDER
 ACTIVECAPTION

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

@@ -0,0 +1,334 @@
+---
+author: Carlos Zamora @carlos-zamora
+created on: 2021-03-12
+last updated: 2021-03-17
+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
+
+`Command` will be updated to look like the following:
+
+```c++
+runtimeclass Command
+{
+    // 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.
+    // NOTE: This is really the only relevant change here.
+    //       We're actually holding the KeyChord instead of just the text.
+    IReference<KeyChord> Keys;
+
+    // The action itself.
+    ActionAndArgs Action;
+
+    // NOTE: nested and iterable command logic will still be here. But they are omitted
+    //       to make this section seem cleaner.
+
+    // Future Considerations:
+    // - [#6899]: Action IDs --> add an identifier 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`.
+ - 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.
+   - Instead, we'll only expose `Keys`, and the caller (i.e. Command Palette) would be responsible for converting that
+      into a legible format.
+ - Observable properties
+   - Several members can be exposed as observable (as they are in `Command`) such as the name, key chord text, and icon.
+   - NOTE: `Command` has observable properties today. Ideally, we could remove `INotifyPropertyChanged` from the settings model
+      entirely, but this will be a longer process. We'll just not rely on this too much for now.
+ - 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 `Command` class is simply being promoted to include the `KeyChord` it has.
+ This allows the implementation cost of this step to be relatively small.
+
+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);
+    ActionAndArgs GetActionByKeyChord(KeyChord keys);
+
+    KeyChord GetKeyBindingForAction(ShortcutAction action);
+    KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs);
+
+    // Future Considerations:
+    // - [#6899]: Action IDs --> GetActionByID()
+}
+```
+
+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).
+See [Copying the `ActionMap`](#copying-the-actionmap) to learn more about how this system works when creating
+ a clone of the settings model.
+
+### 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` may 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 `ActionMap`](#copying-the-actionmap) in the potential issues.
+3. Modifying the `Action`s
+    - `ActionMap` must be responsible for changing `Action`s:
+        - If `Name` changes, update `_NameMap`
+        - If `Keys` changes, update `_KeyMap`
+        - Also update the `Action` itself
+    - This is similar to how color schemes are maintained today.
+    - 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. See [Modifying Actions](#modifying-actions)
+       in potential issues.
+4. Serialization
+    - `Action::ToJson()` and `ActionMap::ToJson()` should perform most of the work for us.
+    - See [Unbinding actions](#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;
+
+// This could be stored as a `map<unsigned int, Action>`,
+//  but a vector provides this functionality much better.
+std::vector<Action> _ActionList;
+```
+
+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 `_ActionList`.
+
+Now that there are no duplicates between `_NameMap` and `_KeyMap`, creating a copy of `_NameMap` and
+ `_KeyMap` is trivial. We can iterate over `_ActionList` to create a copy of every `Action` stored
+ and save it to a new `_ActionList`.
+
+### Layering Actions
+
+We need a way to determine where an action came from to minimize how many actions we serialize when we
+ write to disk. This is a two part approach that happens as we're loading the settings
+ 1. Load defaults.json
+     - For each of the actions in the JSON...
+         - Construct the `Command` (basically the `Command::LayerJson` we have today)
+         - Add it to the `ActionMap`
+           - this should update `_NameMap`, `_KeyMap`, and `_ActionList` appropriately
+           - if the newly added name/key chord conflicts with a pre-existing one,
+              redirect `_NameMap` or `_KeyMap` to the newly added `Command` instead,
+              and update the conflicting one.
+2. Load settings.json
+     - Add a parent to `ActionMap`
+         - The purpose of a parent is to continue a search when the current `ActionMap`
+            can't find a `Command` for a query. The parent is intended to be immutable.
+     - Load the actions array like normal (see step 1)
+
+Introducing a parent mechanism to `ActionMap` allows it to understand where a `Command`
+ came from. This allows us to minimize the number of actions we serialize when we write
+ to disk, as opposed to serializing the entire list of actions.
+
+`ActionMap` queries will need to check their parent when they cannot find a matching `Command`
+ in their `_ActionList` (or sooner).
+
+### Modifying Actions
+
+There are several ways a command can be modified:
+- change/remove the key chord
+- change the name
+- change the icon
+- change the action
+
+It is important that these modifications are done through `ActionMap` instead of `Command`.
+ This is to ensure that the `ActionMap` is always aligned with `Command`'s values. Thus,
+ we can add the following functions to `ActionMap`:
+
+ ```c++
+ runtimeclass ActionMap
+{
+    void SetKeyChord(Command cmd, KeyChord keys);
+    void SetName(Command cmd, String name);
+    void SetIcon(Command cmd, String iconPath);
+    void SetAction(Command cmd, ShortcutAction action, IActionArgs actionArgs);
+}
+ ```
+
+`SetKeyChord` will need to make sure to modify the `_KeyMap` and the provided `Command`.
+ If the new key chord was already taken, we also need to update the conflicting `Command`
+ and remove its key chord.
+
+`SetName` will need to make sure to modify the `_NameMap` and the provided `Command`.
+ We also need to iterate through the list of commands and see if the old name is still
+ in use by another command. If that's the case, don't remove `_NameMap[oldName]`,
+ instead retarget it.
+
+`SetIcon` will only need to modify the provided `Command`.
+
+`SetAction` will need to begin by updating the provided `Command`'s `ActionAndArgs`.
+ If the generated name is being used, the name will need to be updated. As with `SetName`,
+ we also need to update `_NameMap` and check if the old name is still in use by another command.
+
+Regarding [Layering Actions](#layering-actions), if the `Command` does not exist in the current layer,
+ but exists in a parent layer, we need to...
+ 1. duplicate it
+ 2. store the duplicate in the current layer
+ 3. make the modification to the duplicate
+
+This ensures that the change propagates post-serialization.
+
+### Unbinding actions
+
+Removing a name or a key chord is currently omitted from this spec because there
+ is no Settings UI use case for it at the moment. This class of scenarios is
+ designed for Command Palette customization.
+
+The only kind of unbinding currently in scope is freeing a key chord such that
+ no action is executed on that key stroke. To do this...
+ 1. On the existing `Action`, set `ShortcutAction = Unbound` and `IActionArgs = nullptr`
+ 2. The associated name and `KeyChord` are mapped to this "unbound" action
+
+In explicitly storing an "unbound" action, we are explicitly saying that this key chord
+ must be passed through and this string must be removed from the command palette.
+
+We specifically need an "unbound" action for serialization. This way, we can output something like this
+ in the JSON:
+
+```js
+{ "command": "unbound", "keys": "ctrl+c" }
+```
+
+
+## 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.
+
+## 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