Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Spec] Settings Model - Actions #9428

Merged
9 commits merged into from
May 5, 2021
Merged

[Spec] Settings Model - Actions #9428

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3905c49
[Spec] Settings Model - Actions
carlos-zamora Mar 9, 2021
40f817b
maybe this'll fix the references?
carlos-zamora Mar 12, 2021
f1efe1a
apply feedback from spec meeting & comments
carlos-zamora Mar 17, 2021
75f486b
spell check
carlos-zamora Mar 17, 2021
de7cba9
Apply the lessons I've learned
carlos-zamora Apr 27, 2021
71060b9
apply PR feedback
carlos-zamora Apr 27, 2021
71ee4ca
resolve PR comments from Dustin
carlos-zamora Apr 28, 2021
02dd1b8
consolidated actions
carlos-zamora May 3, 2021
0485a87
spell fix
carlos-zamora May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions doc/specs/#885 - Terminal Settings Model/Actions Addendum.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<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.
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:
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<InternalActionID>`)

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. x is a nested command:
    • update _NestedCommands such that "Split Pane..." maps to x
  2. x is a standard command:
    • remove Split Pane... from _NestedCommands
    • register x with _ActionMap

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.

  1. 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 _NestedCommands are 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 NameMap filled 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...
  2. 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.


### Modifying Actions

Expand Down Expand Up @@ -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
Expand Down