20 KiB

Raw Permalink Blame History

author: Carlos Zamora @carlos-zamora created on: 2021-03-12 last updated: 2021-03-17 issue id: #885

Actions in the Settings Model

Abstract

This spec proposes a refactor of how Windows Terminal actions are stored in the settings model. The new representation would mainly allow the serialization and deserialization of commands and keybindings.

Inspiration

A major component that is missing from the Settings UI is the representation of keybindings and commands. The JSON represents both of these as a combined entry as follows:

{ "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:

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.
Handling Duplicates
- The same action can be bound to multiple key chords. The command palette combines all of these actions into one entry because they have the same name. In reality, this same action is just being referenced in different ways.

Solution Design

I propose that the issues stated above be handled via the following approach.

Step 1: Consolidating actions

Command will be updated to look like the following:

runtimeclass Command
{
    // The path to the icon (or icon itself, if it's an emoji)
    String IconPath;

    // The associated name. If none is defined, one is auto-generated.
    String Name;

    // The key binding that can be used to invoke this action.
    // NOTE: 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 ActionAndArgs;

    // NOTE: nested and iterable command logic will still be here,
    //       But they are omitted to make this section seem cleaner.

    // Future Considerations:
    // - [#6899]: Action IDs --> add an identifier here
}

The goal here is to consolidate key binding actions and command palette actions into a single class. This will also require the following supplemental changes:

Command::LayerJson
- This must combine the logic of KeyMapping::LayerJson and Command::LayerJson.
Key Chord data
- Internally, store a vector<KeyChord> _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
- 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.

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 KeyChords to ActionAndArgs. This leads us to step 2 of this process.

Step 2: Querying actions

Key bindings and commands are deserialized by basically storing individual actions to a map.

KeyMapping is basically an IMap<KeyChord, ActionAndArgs> with a few extra functions. In fact, it actually stores key binding data to a std::map<KeyChord, ActionAndArgs> and directly interacts with it.
Command::LayerJson populates an IMap<String, Command> during deserialization as it iterates over every action. Note that Command can be interpreted as a wrapper for ActionAndArgs with more stuff here.

It makes sense to store these actions as maps. So, following step 1 above, we can also store and expose actions something like the following:

runtimeclass ActionMap
{
    ActionAndArgs GetActionByKeyChord(KeyChord keys);

    KeyChord GetKeyBindingForAction(ShortcutAction action);
    KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs);

    IMapView<String, Command> NameMap { get; };

    // Future Considerations:
    // - [#6899]: Action IDs --> GetActionByID()
}

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:

std::map<KeyChord, InternalActionID> _KeyMap;
std::map<InternalActionID, Command> _ActionMap;

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

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:

Exposing the maps
- ActionMap::KeyBindings and ActionMap::Commands may need to be added to pass the full list of actions to the Settings UI.
- In doing this, we can already update the Settings UI to include a better view of our actions.
Creating a copy of the settings model
- The Settings UI operates by binding the XAML controls to a copy of the settings model.
- 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.
Modifying the Commands
- ActionMap must be responsible for changing Commands 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 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
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 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

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

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 the internal state of ActionMap appropriately
    - if the newly added key chord conflicts with a preexisting one, redirect _KeyMap to the newly added Command instead, and update the conflicting one.
Load settings.json
- 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 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.

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 use a std::set<InternalActionID> 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:

Get an accumulated list of Commands from our parents
Iterate over the list...
- Update NameMap with any new Commands (tracked by the std::set<InternalActionID>)

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 Commands 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 Commands. 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 Commands 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

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. Command should only expose getters in the projected type to enforce this. Thus, we can add the following functions to ActionMap:

runtimeclass ActionMap
{
   void SetKeyChord(Command cmd, KeyChord keys);
   void SetName(Command cmd, String name);
   void SetIcon(Command cmd, String iconPath);
   void SetAction(Command cmd, ShortcutAction action, IActionArgs actionArgs);
}

SetKeyChord will need to make sure to modify the _KeyMap and the provided Command. If the new key chord was already taken, we also need to update the conflicting Command and remove its key chord.

SetName will need to make sure to modify the Command in _ActionMap and regenerate NameMap.

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. _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, if the Command does not exist in the current layer, but exists in a parent layer, we need to... 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.

duplicate it with Command::Copy
store the duplicate in the current layer
- ActionMap::AddAction(duplicate)
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 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, simply ActionMap::AddAction a Command with...

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 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.

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 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

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 earlier, we would simply introduce a new std:map<string, InternalActionID> _ExternalIDMap that is updated like the others, and add a String ID property to Action.
#8100 Source Tracking
- Identifying where a setting came from can be very beneficial in the settings model and UI. For example, profile settings now have an OverrideSource getter that describes what Profile object the setting came from (i.e. base layer, profile generator, etc...). A similar system can be used for Action in that we record if the action was last modified in defaults.json or settings.json.
- There seems to be no desire for action inheritance (i.e. inheriting the name/key-chord from the parent). So this should be sufficient.

Resources

Other references: [Settings UI: Actions Page]: https://github.com/microsoft/terminal/issues/6900

20 KiB Raw Permalink Blame History