[Spec] Settings Model - Actions #9428
Conversation
carlos-zamora
added
Issue-Docs
|
|
||
| `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). | ||
|
|
There was a problem hiding this comment.
Should we mention layering here too? Because the layering more applies to the container of objects than it does the individual Actions. Like, when you have
{ "keys": "ctrl+a", "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}then we'll need to first set the KeyChordText of the copy action to "ctrl+a", but then when we go to replace copy in the keymap with paste, we'll need to clear that copy's KeyChordText when we replace it with paste
There was a problem hiding this comment.
Added a layering section for the defaults.json and settings.json layering.
In an event like you described above where both occur in the same layer, my plan is to keep "copy" in the action list, but actually have "paste" be bound to ctrl+a. Serialization-wise, we'll only output:
{ "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}I think this is what our current actions model does, because we just iterate over all of our actions and add them to the command palette independently of their key chord.
|
|
||
| Removing a name or a key chord from an `Action` propagates the change to the `ActionMap`. | ||
|
|
||
| Removing the action itself is more complicated... |
There was a problem hiding this comment.
Wait could you elaborate more on how we get into the "remove an action" state? Like, removing the keys from an action, I get why we'd do that. I get why we'd set the name of an action to null. But why are we replacing an action with Unbound? Like, what's the user story here?
There was a problem hiding this comment.
We talked a bit about this in the spec meeting, and I've updated the spec to kinda cover this a bit better. Let me know if it's still not clear though :)
Notes from Spec Meeting (3/16)
Spec needs some examples of layering. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Curious to see how https://github.com/microsoft/terminal/pull/9428/files#r598999437 is resolved, but I think this is otherwise fine?
(there's no other approvals ATM, so I'm gonna ✔️ this so GitHub remembers my place, and because I don't have blocking concerns anymore)
| - 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. |
There was a problem hiding this comment.
So to be clear - "update the conflicting one" means something like "If a subsequent command has the same Keys as an existing one, we'll remove the Keys from the existing one, then update the _KeyMap to point at the subsequent command (instead of the original)"
For Names, we don't really need to remove the Name from the old command in the _NameMap, right?
There was a problem hiding this comment.
I removed _NameMap from the spec so resolving.
There was a problem hiding this comment.
NameMap is still in the spec in the public interface for ActionMap..?
There was a problem hiding this comment.
Yeah, we still need a NameMap exposed to the projected type for the Command Palette. There's no (easy) way around that because the command palette needs to expand the iterable commands over in TermApp. So the process looks something like this:
- TermApp requests ActionMap::NameMap
- we generate the NameMap (as described in the spec)
- TermApp expands any iterable commands
- TermApp stores this expanded NameMap and uses it whenever the Command Palette requests it
Internally (to ActionMap), we won't be updating a _NameMap as we add more actions. It's just not worth it because we only want each action to appear once (and the _ActionMap already keeps track of that).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| Since `NameMap` is generated upon request, we will need to pass a `std::set<InternalActionID>` 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. |
There was a problem hiding this comment.
do nested commands have a special hash function that ensures that they will not collide with a hash generated from an Action+ActionAndArgs
There was a problem hiding this comment.
or do all nested commands hash to the same thing
There was a problem hiding this comment.
Added a section in the spec. The gist of it is...
- No, nested commands can only be identified by their given name. Thus, we cannot add them to the internal
_ActionMap. Instead, we add them to their ownmap<hstring, Command> _NestedCommands. - Handling collisions:
- as actions get added to the
ActionMap, add nested commands to their own_NestedCommandsmap. - If a newly added
Commandconflicts with a name in_NestedCommands, resolve in favor of the newCommandby removing the conflicting nested command from_NestedCommands.
- as actions get added to the
ghost
added
Needs-Author-Feedback
This comment has been minimized.
This comment has been minimized.
| 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. |
There was a problem hiding this comment.
I'm still worried about this.
Consider that you have the following nested/repeated commands:
Split Pane...
Windows PowerShell
New Tab...
Windows PowerShell
those have the same name. They are very much different commands. We're not keying anything off their names, right? Well, except the Name Map which will immediately and horribly explode when you do that.
There was a problem hiding this comment.
I think there seems to be some confusion from my word choice. When I say "nested commands", I always mean the root. So in your example, there are 2 nested commands:
- "split pane..."
- "new tab..."
Part 1: AddAction the nested command
AddAction receives these commands and does the following:
- is it a nested command?
- Yes! Insert (or assign) "" to
_NestedCommands
- Yes! Insert (or assign) "" to
- Done!
Part 2: AddAction a conflicting command
Later, let's say we get a new Command x who's name is "Split Pane...". We have two paths here:
xis a nested command:- update
_NestedCommandssuch that "Split Pane..." maps tox
- update
xis a standard command:- remove
Split Pane...from_NestedCommands - register
xwith_ActionMap
- remove
NOTE: We don't care what is inside of these nested commands. We just care about the root.
Part 3: NameMap generation with our parents
Now we get to part 3 of our story: dealing with parents. We don't really care about our parents until we have to generate the NameMap.
- add all of the nested actions
- Due to part 2, we can assume that we have a disjoint set of names between standard and nested actions. This means that all names in
_NestedCommandsare new to those from the standard actions. - If we start from our parents' nested actions, then work our way down to the current layer, we can create a
NameMapfilled with just nested commands. We don't know if any of them will persist, but that's ok, because then we move on to step 2...
- Due to part 2, we can assume that we have a disjoint set of names between standard and nested actions. This means that all names in
- add all of the standard/consolidated actions
- ANY collisions are just straight-up assigned to these actions. The only collisions we can expect are the ones from our ancestors' nested commands intersecting with our standard actions. If that happens, we have the right to prioritize our standard actions.
|
I'm gonna go out on a limb and say that because this spec isn't for any sort of external-facing feature, we might be able to roll with just two signoffs. As always, the code is truth, so this is more an explanation of the thought process behind #9621 than anything else. |
This comment has been minimized.
This comment has been minimized.
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit This entirely removes `KeyMapping` from the settings model, and builds on the work done in #9543 to consolidate all actions (key bindings and commands) into a unified data structure (`ActionMap`). ## References #9428 - Spec #6900 - Actions page Closes #7441 ## Detailed Description of the Pull Request / Additional comments The important thing here is to remember that we're shifting our philosophy of how to interact/represent actions. Prior to this, the actions arrays in the JSON would be deserialized twice: once for key bindings, and again for commands. By thinking of every entry in the relevant JSON as a `Command`, we can remove a lot of the context switching between working with a key binding vs a command palette item. #9543 allows us to make that shift. Given the work in that PR, we can now deserialize all of the relevant information from each JSON action item. This allows us to simplify `ActionMap::FromJson` to simply iterate over each JSON action item, deserialize it, and add it to our `ActionMap`. Internally, our `ActionMap` operates as discussed in #9428 by maintaining a `_KeyMap` that points to an action ID, and using that action ID to retrieve the `Command` from the `_ActionMap`. Adding actions to the `ActionMap` automatically accounts for name/key-chord collisions. A `NameMap` can be constructed when requested; this is for the Command Palette. Querying the `ActionMap` is fairly straightforward. Helper functions were needed to be able to distinguish an explicit unbinding vs the command not being found in the current layer. Internally, we store explicitly unbound names/key-chords as `ShortcutAction::Invalid` commands. However, we return `nullptr` when a query points to an unbound command. This is done to hide this complexity away from any caller. The command palette still needs special handling for nested and iterable commands. Thankfully, the expansion of iterable commands is performed on an `IMapView`, so we can just expose `NameMap` as a consolidation of `ActionMap`'s `NameMap` with its parents. The same can be said for exposing key chords in nested commands. ## Validation Steps Performed All local tests pass.
## Summary of the Pull Request This PR lays the foundation for a new Actions page in the Settings UI as designed in #6900. The Actions page now leverages the `ActionMap` to display all of the key bindings and allow the user to modify the associated key chord or delete the key binding entirely. ## References #9621 - ActionMap #9926 - ActionMap serialization #9428 - ActionMap Spec #6900 - Actions page #9427 - Actions page design doc ## Detailed Description of the Pull Request / Additional comments ### Settings Model Changes - `Command::Copy()` now copies the `ActionAndArgs` - `ActionMap::RebindKeys()` handles changing the key chord of a key binding. If a conflict occurs, the conflicting key chord is overwritten. - `ActionMap::DeleteKeyBinding()` "deletes" a key binding by binding "unbound" to the given key chord. - `ActionMap::KeyBindings()` presents another view (similar to `NameMap`) of the `ActionMap`. It specifically presents a map of key chords to commands. It is generated similar to how `NameMap` is generated. ### Editor Changes - `Actions.xaml` is mainly split into two parts: - `ListView` (as before) holds the list of key bindings. We _could_ explore the idea of an items repeater, but the `ListView` seems to provide some niceties with regards to navigating the list via the key board (though none are selectable). - `DataTemplate` is used to represent each key binding inside the `ListView`. This is tricky because it is bound to a `KeyBindingViewModel` which must provide _all_ context necessary to modify the UI and the settings model. We cannot use names to target UI elements inside this template, so we must make the view model smart and force updates to the UI via changes in the view model. - `KeyBindingViewModel` is a view model object that controls the UI and the settings model. There are a number of TODOs in Actions.cpp will be long-term follow-ups and would be nice to have. This includes... - a binary search by name on `Actions::KeyBindingList` - presenting an error when the provided key chord is invalid. ## Demo 
This spec covers the settings model work required to create the Actions page in the settings UI (designed in #9427).
Overall, the idea is to promote
Commandto include the actualKeyChord, then introduce anActionMapthat handles all of the responsibilities ofKeyMappingand more (as well as general action management).Markdown view