From 3905c49ca764cebced478ef310aed9f78aa842f2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 9 Mar 2021 11:06:33 -0800 Subject: [PATCH 1/9] [Spec] Settings Model - Actions --- .../Actions Addendum.md | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 doc/specs/#885 - Terminal Settings Model/Actions Addendum.md diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md new file mode 100644 index 00000000000..3dc39bb8f39 --- /dev/null +++ b/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 ctrl+c +- 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 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` with a few extra functions. In fact, it actually + stores key binding data to a `std::map` and directly interacts with it. +- `Command::LayerJson` populates an `IMap` 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 _NameMap; +std::map _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 _NameMap; +std::map _KeyMap; +std::map _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 _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 From 40f817b38f22c92a6971993df741407ae27ff96d Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 11 Mar 2021 16:43:40 -0800 Subject: [PATCH 2/9] maybe this'll fix the references? --- .../#885 - Terminal Settings Model/Actions Addendum.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 3dc39bb8f39..8b10e4809bb 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -63,7 +63,7 @@ runtimeclass Action IActionArgs Args; // Future Considerations: - // - #6899: Action IDs --> add an identifier here + // - [#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 } @@ -114,7 +114,7 @@ runtimeclass ActionMap KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs); // Future Considerations: - // - #6899: Action IDs --> GetActionByID() + // - [#6899]: Action IDs --> GetActionByID() // - Getters for groups of actions... // - IMap<> GetActionsByGrouping // - IMap<> GetActionsByTag @@ -233,13 +233,13 @@ In making a distinction between "unbound" actions and "invalid" actions, we can ## Future considerations There are a number of ideas regarding actions that would be fairly trivial to implement given this refactor: -- #6899: Action IDs +- [#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 _ExternalIDMap` that is updated like the others, and add a `String ID` property to `Action`. -- #8100 Source Tracking +- [#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 @@ -254,7 +254,7 @@ There are a number of ideas regarding actions that would be fairly trivial to im 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. + for [#8767], and allow for window-level actions to be accessed via the command palette and key bindings. ## Resources From f1efe1ae418cc7b66d2002836d7e366ea2281e63 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 16 Mar 2021 20:48:52 -0700 Subject: [PATCH 3/9] apply feedback from spec meeting & comments --- .../Actions Addendum.md | 195 ++++++++++++------ 1 file changed, 130 insertions(+), 65 deletions(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 8b10e4809bb..7e851db1f84 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/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,13 +29,13 @@ 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 @@ -43,10 +43,10 @@ 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,17 +55,18 @@ 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 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 } ``` @@ -73,20 +74,22 @@ The goal here is to consolidate key binding actions and command palette actions 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,6 +131,8 @@ std::map _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 @@ -138,21 +140,24 @@ After the former two steps are completed, the new representation of actions in t 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 _NameMap; std::map _KeyMap; -std::map _IDMap; -unsigned int _nextID = 1; +// This could be stored as a `map`, +// but a vector provides this functionality much better. +std::vector _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 From 75f486ba0421a66fb2e1f48787c15f4c925d29b9 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 17 Mar 2021 13:45:13 -0700 Subject: [PATCH 4/9] spell check --- .github/actions/spelling/expect/expect.txt | 1 + doc/specs/#885 - Terminal Settings Model/Actions Addendum.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 96c6fe7f479..8bed564fb0a 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -18,6 +18,7 @@ ACIOSS ACover actctx ACTCTXW +actionmap activatable ACTIVEBORDER ACTIVECAPTION diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 7e851db1f84..083d9cb0114 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -282,7 +282,7 @@ 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 +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. From de7cba9805e91a70e0b364ae2a8844131f6eb7e4 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 27 Apr 2021 12:01:36 -0700 Subject: [PATCH 5/9] Apply the lessons I've learned --- .../Actions Addendum.md | 143 ++++++++---------- 1 file changed, 61 insertions(+), 82 deletions(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 083d9cb0114..c97639037b0 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -55,15 +55,16 @@ runtimeclass Command 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 Keys; + // NOTE: We're actually holding the KeyChord instead of just the text. + // KeyChordText just serializes the relevant keychord + Microsoft.Terminal.Control.KeyChord Keys; + String KeyChordText; // The action itself. - ActionAndArgs Action; + ActionAndArgs ActionAndArgs; - // NOTE: nested and iterable command logic will still be here. But they are omitted - // to make this section seem cleaner. + // 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 @@ -72,21 +73,21 @@ runtimeclass Command 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` + - `Command::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. + - Key Chord data + - Internally, store a `vector _keyMappings` to keep track of all the key chords associated with this action. + - `RegisterKey` and `EraseKey` update `_keyMappings`, and ensure that the latest key registered is at the end of the list. + - `Keys()` simply returns the last entry of `_keyMappings`, which is the latest key chord this action is bound to. + - `KeyChordText())` is exposed to pass the text directly to the command palette. + This depends 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. - - 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. + - `Command` has observable properties today, but does not need them because `Command` will never change while the app is running. - 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. + - An additional `IsNestedCommand` is necessary to record a case where a nested command is being unbound `{ "commands": null, "name": "foo" }`. 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. @@ -110,29 +111,33 @@ It makes sense to store these actions as maps. So, following step 1 above, we ca ```c++ runtimeclass ActionMap { - Action GetActionByName(String name); ActionAndArgs GetActionByKeyChord(KeyChord keys); KeyChord GetKeyBindingForAction(ShortcutAction action); KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs); + IMapView NameMap { get; }; + // 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: +The getters will return null if a matching action or key chord is not found. Since iterable commands need to be expanded at in TerminalApp, we'll just expose `NameMap`, then let TerminalApp perform the expansion as they do now. Internally, we can store the actions as follows: ```c++ -std::map _NameMap; -std::map _KeyMap; +std::map _KeyMap; +std::map _ActionMap; ``` -`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. +`InternalActionID` will be a hash of `ActionAndArgs` such that two `ActionAndArgs` with the same `ShortcutAction` and `IActionArgs` output the same hash value. + +`GetActionByKeyChord` will use `_KeyMap` to find the `InternalActionID`, then use the `_ActionMap` to find the bound `Command`. +`GetKeyBindingForAction` will hash the provided `ActionAndArgs` (constructed by the given parameters) and check `_ActionMap` for the given `InternalActionID`. + +`NameMap` will need to ensure every action in `_ActionMap` is added to the output name map if it has an associated name. This is done by simply iterating over `_ActionMap`. Nested commands must be added separately because they cannot be hashed. + +`ActionMap` will have an `AddAction(Command cmd)` that will update the internal state whenever a command is registered. If the given command is valid, we will check for collisions and resolve them. Otherwise, we will consider this an "unbound" action and update the internal state normally. It is important that we don't handle "unbound" actions differently because this ensures that we are explicitly unbinding a key chord. ### Step 3: Settings UI needs @@ -140,23 +145,22 @@ After the former two steps are completed, the new representation of actions in t 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. + - `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 + - Copying the `ActionMap` is fairly simple. Just copy the internal state and ensure that `Command::Copy` is called such that no reference to the original WinRT objects persist. Since we are using `InternalActionID`, we will not have to worry about multiple `Command` references existing within the same `ActionMap`. +3. Modifying the `Command`s + - `ActionMap` must be responsible for changing `Command`s so that we can ensure `ActionMap` always has a correct internal state: + - It is important that `Command` only exposes getters (not setters) to ensure `ActionMap` is up to date. + - If a key chord is being changed, update the `_KeyMap` and the `Command` itself. + - If a key binding is being deleted, add an unbound action to the given key chord. - 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. + - `Command::ToJson()` and `ActionMap::ToJson()` should perform most of the work for us. Simply iterate over the `_ActionMap` and call `Command::ToJson` on each action. - See [Unbinding actions](#unbinding-actions) in potential issues. @@ -188,28 +192,6 @@ N/A ## 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 _NameMap; -std::map _KeyMap; - -// This could be stored as a `map`, -// but a vector provides this functionality much better. -std::vector _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 @@ -218,22 +200,22 @@ We need a way to determine where an action came from to minimize how many action - 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, + - this should update the internal state of `ActionMap` appropriately + - if the newly added key chord conflicts with a pre-existing one, + redirect `_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. + - 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. +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). +`ActionMap` queries will need to check their parent when they cannot find a matching `InternalActionID` in their `_ActionMap`. + +Since `NameMap` is generated upon request, we will need to pass a `std::set` as we generate the `NameMap` across each layer. This will ensure that each `Command` is only added to the `NameMap` once. We will start from the current layer and move up the inheritance tree to ensure that the current layer takes priority. + +Nested commands will be saved to their own map since they do not have an `InternalActionID`. To ensure layering is accomplished correctly, we will need to start from the top-most parent and update `NameMap`. As we go down the inheritance tree, any conflicts are resolved by prioritizing the current layer (the child). This ensures that the current layer takes priority. ### Modifying Actions @@ -243,6 +225,8 @@ There are several ways a command can be modified: - change the icon - change the action +Changing the name and + 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`: @@ -261,16 +245,12 @@ It is important that these modifications are done through `ActionMap` instead of 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. +`SetName` will need to make sure to modify the `Command` in `_ActionMap` and regenerate `NameMap`. `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. + If the generated name is being used, the name will need to be updated. Regarding [Layering Actions](#layering-actions), if the `Command` does not exist in the current layer, but exists in a parent layer, we need to... @@ -278,24 +258,23 @@ Regarding [Layering Actions](#layering-actions), if the `Command` does not exist 2. store the duplicate in the current layer 3. make the modification to the duplicate -This ensures that the change propagates post-serialization. +This ensures that the change persists 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 +Removing a name is currently omitted from this spec because there + is no Settings UI use case for it at the moment. This scenario 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 + no action is executed on that key stroke. To do this, simply `ActionMap::AddAction` a `Command` with... + - `ActionAndArgs`: `ShorctutAction = Invalid` and `IActionArgs = nullptr` + - `Keys` being the provided key chord 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. + must be passed through and this string must be removed from the command palette. `AddAction` automatically handles updating the internal state of `ActionMap` and any conflicting `Commands`. -We specifically need an "unbound" action for serialization. This way, we can output something like this - in the JSON: +This allows us to output something like this in the JSON: ```js { "command": "unbound", "keys": "ctrl+c" } @@ -308,8 +287,8 @@ There are a number of ideas regarding actions that would be fairly trivial to im - [#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 _ExternalIDMap` that is updated like the others, and add a `String ID` + ID system introduced earlier, we would simply introduce a new + `std:map _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, From 71060b98a72ff21088f6ef0bc3af312c0a9375cb Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 27 Apr 2021 12:11:01 -0700 Subject: [PATCH 6/9] apply PR feedback --- .../Actions Addendum.md | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index c97639037b0..939b94a10c3 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -205,9 +205,9 @@ We need a way to determine where an action came from to minimize how many action redirect `_KeyMap` to the newly added `Command` instead, and update the conflicting one. 2. Load settings.json - - Add a parent to `ActionMap` + - Create a child for the `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) + - Load the actions array like normal into the child (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. @@ -225,11 +225,8 @@ There are several ways a command can be modified: - change the icon - change the action -Changing the name and - 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`: + This is to ensure that the `ActionMap` is always aligned with `Command`'s values. `Command` should only expose getters in the projected type to enforce this. Thus, we can add the following functions to `ActionMap`: ```c++ runtimeclass ActionMap @@ -247,10 +244,10 @@ It is important that these modifications are done through `ActionMap` instead of `SetName` will need to make sure to modify the `Command` in `_ActionMap` and regenerate `NameMap`. -`SetIcon` will only need to modify the provided `Command`. +`SetIcon` will only need to modify the provided `Command`. We can choose to not expose this in the `ActionMap`, but doing so makes the API consistent. `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. + If the generated name is being used, the name will need to be updated. `_ActionMap` will need to be updated with a new `InternalActionID` for the new action. This is a major operation and so all views exposed will need to be regenerated. Regarding [Layering Actions](#layering-actions), if the `Command` does not exist in the current layer, but exists in a parent layer, we need to... @@ -268,7 +265,7 @@ Removing a name is currently omitted from this spec because there 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, simply `ActionMap::AddAction` a `Command` with... - - `ActionAndArgs`: `ShorctutAction = Invalid` and `IActionArgs = nullptr` + - `ActionAndArgs`: `ShortcutAction = Invalid` and `IActionArgs = nullptr` - `Keys` being the provided key chord In explicitly storing an "unbound" action, we are explicitly saying that this key chord From 71ee4ca8f80c3bddc7dd95434d2fcba0da6c38ee Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 28 Apr 2021 12:59:09 -0700 Subject: [PATCH 7/9] resolve PR comments from Dustin --- .../Actions Addendum.md | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 939b94a10c3..b7c7aa29e08 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -159,6 +159,9 @@ After the former two steps are completed, the new representation of actions in t - 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. + - For the purposes of the key bindings page, we will introduce a `KeyBindingViewModel` to serve as an intermediator between the settings UI and the settings model. The view model will be responsible for things like... + - exposing relevant information to the UI controls + - converting UI control interactions into proper API calls to the settings model 4. Serialization - `Command::ToJson()` and `ActionMap::ToJson()` should perform most of the work for us. Simply iterate over the `_ActionMap` and call `Command::ToJson` on each action. - See [Unbinding actions](#unbinding-actions) in potential issues. @@ -213,9 +216,15 @@ Introducing a parent mechanism to `ActionMap` allows it to understand where a `C `ActionMap` queries will need to check their parent when they cannot find a matching `InternalActionID` in their `_ActionMap`. -Since `NameMap` is generated upon request, we will need to pass a `std::set` as we generate the `NameMap` across each layer. This will ensure that each `Command` is only added to the `NameMap` once. We will start from the current layer and move up the inheritance tree to ensure that the current layer takes priority. +Since `NameMap` is generated upon request, we will need to use a `std::set` as we generate the `NameMap`. This will ensure that each `Command` is only added to the `NameMap` once. The name map will be generated as follows: +1. Get an accumulated list of `Command`s from our parents +2. Iterate over the list... + - Update `NameMap` with any new `Command`s (tracked by the `std::set`) -Nested commands will be saved to their own map since they do not have an `InternalActionID`. To ensure layering is accomplished correctly, we will need to start from the top-most parent and update `NameMap`. As we go down the inheritance tree, any conflicts are resolved by prioritizing the current layer (the child). This ensures that the current layer takes priority. +Nested commands will be saved to their own map since they do not have an `InternalActionID`. +- During `ActionMap`'s population, we must ensure to resolve any conflicts immediately. This means that any new `Command`s that generate a name conflicting with a nested command will take priority (and we'll remove the nested comand from its own map). Conversely, if a new nested command conflicts with an existing standard `Command`, we can ignore it because our generation of `NameMap` will handle it. +- When populating `NameMap`, we must first add all of the standard `Command`s. To ensure layering is accomplished correctly, we will need to start from the top-most parent and update `NameMap`. As we go down the inheritance tree, any conflicts are resolved by prioritizing the current layer (the child). This ensures that the current layer takes priority. +- After adding all of the standard `Command`s to the `NameMap`, we can then register all of the nested commands. Since nested commands have no identifier other than a name, we cannot use the `InternalActionID` heuristic. However, as mentioned earlier, we are updating our internal nested command map as we add new actions. So when we are generating the name map, we can assume that all of these nested commands now have priority. Thus, we simply add all of these nested commands to the name map. Any conflicts are resolved in favor of th nested command. ### Modifying Actions @@ -251,12 +260,18 @@ It is important that these modifications are done through `ActionMap` instead of 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 + 0. check if it exists + - use the hash `InternalActionID` to see if it exists in the current layer + - if it doesn't (which is the case we're trying to solve here), call `_GetActionByID(InternalActionID)` to retrieve the `Command` wherever it may be. This helper function simply checks the current layer, if none is found, it recursively checks its parents until a match is found. + 1. duplicate it with `Command::Copy` 2. store the duplicate in the current layer + - `ActionMap::AddAction(duplicate)` 3. make the modification to the duplicate This ensures that the change persists post-serialization. +TerminalApp has no reason to ever call these setters. To ensure that relationship, we will introduce an `IActionMapView` interface that will only expose `ActionMap` query functions. Conversely, `ActionMap` will be exposed to the TerminalSettingsEditor to allow for modifications. + ### Unbinding actions Removing a name is currently omitted from this spec because there From 02dd1b8bb70c2cbb638e74fca25e345c955b51bb Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 3 May 2021 15:48:09 -0700 Subject: [PATCH 8/9] consolidated actions --- .../Actions Addendum.md | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index b7c7aa29e08..66e0c5f841c 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -222,7 +222,7 @@ Since `NameMap` is generated upon request, we will need to use a `std::set`) Nested commands will be saved to their own map since they do not have an `InternalActionID`. -- During `ActionMap`'s population, we must ensure to resolve any conflicts immediately. This means that any new `Command`s that generate a name conflicting with a nested command will take priority (and we'll remove the nested comand from its own map). Conversely, if a new nested command conflicts with an existing standard `Command`, we can ignore it because our generation of `NameMap` will handle it. +- During `ActionMap`'s population, we must ensure to resolve any conflicts immediately. This means that any new `Command`s that generate a name conflicting with a nested command will take priority (and we'll remove the nested command from its own map). Conversely, if a new nested command conflicts with an existing standard `Command`, we can ignore it because our generation of `NameMap` will handle it. - When populating `NameMap`, we must first add all of the standard `Command`s. To ensure layering is accomplished correctly, we will need to start from the top-most parent and update `NameMap`. As we go down the inheritance tree, any conflicts are resolved by prioritizing the current layer (the child). This ensures that the current layer takes priority. - After adding all of the standard `Command`s to the `NameMap`, we can then register all of the nested commands. Since nested commands have no identifier other than a name, we cannot use the `InternalActionID` heuristic. However, as mentioned earlier, we are updating our internal nested command map as we add new actions. So when we are generating the name map, we can assume that all of these nested commands now have priority. Thus, we simply add all of these nested commands to the name map. Any conflicts are resolved in favor of th nested command. @@ -292,6 +292,25 @@ This allows us to output something like this in the JSON: { "command": "unbound", "keys": "ctrl+c" } ``` +### Consolidated Actions + +`AddAction` must be a bit smarter when it comes to the following scenario: +- Given a command that unbinds a key chord: `{ "command": "unbound", "keys": "ctrl+c" }` +- And... that key chord was set in a parent layer `{ "command": "copy", "keys": "ctrl+c" }` +- But... the action has another key chord from a parent layer `{ "command": "copy", "keys": "ctrl+shift+c" }` + +`_ActionMap` does not contain any information about a parent layer; it only contains actions introduced in the current layer. Thus, in the scenario above, unbinding `ctrl+c` is what is relevant to `_ActionMap`. However, this may cause some complications for `GetKeyChordForAction`. We cannot simply check our internal `_ActionMap`, because the primary key chord in the entry may be incorrect. Again, this is because `_ActionMap` is only aware of what was bound in the current layer. + +To get around this issue, we've introduced `_ConsolidatedActions`. In a way, `_ConsolidatedActions` is similar to `_ActionMap`, except that it consolidates the `Command` data into one entry constructed across the current layer and the parent layers. Specifically, in the scenario above, `_ActionMap` will say that `copy` has no key chords. In fact, `_ActionMap` has no reason to have `copy` at all, because it was not introduced in this layer. Conversely, `_ConsolidatedActions` holds `copy` with a `ctrl+shift+c` binding, which is then returned to `GetKeyChordForAction`. + +To maintain `_ConsolidatedActions`, any new action added to the Action Map must also update `_ConsolidatedActions`. It is especially important to handle and propagate collisions to `_ConsolidatedActions`. + +When querying Action Map for an ID, we should always check in the following order: +- `_ConsolidatedActions` +- `_ActionMap` +- repeat this process for each parent + +This is to ensure that we are returning the correct and wholistic view of a `Command` on a query. Rather than aquiring a `Command` constructed in this layer, we receive one that contains all of the data aquired across the entire Action Map and its parents. ## Future considerations From 0485a8739cabdbc78fff180f8447a9a6979add48 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 4 May 2021 11:39:39 -0700 Subject: [PATCH 9/9] spell fix --- doc/specs/#885 - Terminal Settings Model/Actions Addendum.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md index 66e0c5f841c..d7230dd6c37 100644 --- a/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md +++ b/doc/specs/#885 - Terminal Settings Model/Actions Addendum.md @@ -310,7 +310,7 @@ When querying Action Map for an ID, we should always check in the following orde - `_ActionMap` - repeat this process for each parent -This is to ensure that we are returning the correct and wholistic view of a `Command` on a query. Rather than aquiring a `Command` constructed in this layer, we receive one that contains all of the data aquired across the entire Action Map and its parents. +This is to ensure that we are returning the correct and wholistic view of a `Command` on a query. Rather than acquiring a `Command` constructed in this layer, we receive one that contains all of the data acquired across the entire Action Map and its parents. ## Future considerations