apply feedback from spec meeting & comments

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

@@ -1,7 +1,7 @@
 ---
 author: Carlos Zamora @carlos-zamora
 created on: 2021-03-12
-last updated: 2021-03-12
+last updated: 2021-03-17
 issue id: [#885]
 ---
 
@@ -29,24 +29,24 @@ However, at the time of writing this spec, the settings model represents it as..
 
 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.
+    - 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.
+    - 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:
+`Command` will be updated to look like the following:
 
 ```c++
-runtimeclass Action
+runtimeclass Command
 {
     // The path to the icon (or icon itself, if it's an emoji)
     String IconPath;
@@ -55,38 +55,41 @@ runtimeclass Action
     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.
-    // NOTE: This can be represented as an ActionAndArgs to simplify the work that needs to be done.
-    ShortcutAction Command;
-    IActionArgs Args;
+    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
-    // - 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.
+   - `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 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.
+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
@@ -108,16 +111,13 @@ It makes sense to store these actions as maps. So, following step 1 above, we ca
 runtimeclass ActionMap
 {
     Action GetActionByName(String name);
-    Action GetActionByKeyChord(KeyChord keys);
+    ActionAndArgs 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
 }
 ```
 
@@ -131,28 +131,33 @@ 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` 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.
+    - `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 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.
+    - 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" in potential issues.
+    - `Action::ToJson()` and `ActionMap::ToJson()` should perform most of the work for us.
+    - See [Unbinding actions](#unbinding-actions) in potential issues.
 
 
 ## UI/UX Design
@@ -192,43 +197,110 @@ structures. To get around this issue, we can introduce internal action IDs to en
 ```c++
 std::map<std::wstring, unsigned int> _NameMap;
 std::map<KeyChord, unsigned int> _KeyMap;
-std::map<unsigned int, Action> _IDMap;
 
-unsigned int _nextID = 1;
+// 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 `_IDMap` and increment `_nextID`.
+ 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 `_IDMap` to create a copy of every `Action` stored
- and save it to a new `_IDMap`.
+ `_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);
+}
+ ```
 
-### Unbinding actions
+`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.
 
-Removing a name or a key chord from an `Action` propagates the change to the `ActionMap`.
+`SetIcon` will only need to modify the provided `Command`.
 
-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
+`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.
 
-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.
+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
 
-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.
+This ensures that the change propagates post-serialization.
+
+### Unbinding actions
+
+Removing a name or a key chord is currently ommitted 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": "false" }
+{ "command": "unbound", "keys": "ctrl+c" }
 ```
 
-In making a distinction between "unbound" actions and "invalid" actions, we can theoretically also
- serialize invalid actions.
 
 ## Future considerations
 
@@ -245,16 +317,7 @@ There are a number of ideas regarding actions that would be fairly trivial to im
      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.
+     So this should be sufficient.
 
 ## Resources
 
@@ -265,5 +328,7 @@ There are a number of ideas regarding actions that would be fairly trivial to im
 
 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