Applies feedback from https://github.com/microsoft/terminal/pull/9949#pullrequestreview-662590658
Highlights include:
- bugfix: make all edit buttons stay visible if the user is using assistive technology
- rename a few functions and resources to match the correct naming scheme
- update the localized text for a conflicting key chord being assigned
- provide better comments throughout the actions page code
## References
#9949 - Original PR
Closes#10168
## 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 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.
As mentioned in https://github.com/microsoft/terminal/issues/9354#issuecomment-790034728
`GETSET_SETTING` is too visually similar to `GETSET_PROPERTY`, but with a _VERY_ different meaning. I think that merely changing the name of the macro would make it harder for us to make this mistake again.
This was the only thing blocking me from signing off on #9224 in 1.7.
! CHANGE WARNING !
If we bind to `T.S.M.Command`s in XAML, then the compiler gets _very
angry_ at us. It generates two different versions of
`GetReferenceTypeMember_Icon` in `XamlTypeInfo.g.cpp`. Presumably
because there's an Icon on a NavViewItem and an Icon on a Command. We
don't really know why. Fortunately, the fix is "rename Command::Icon" to
"Command::IconPath". It's dumb, but it works. Thanks for the help with
that one Carlos ☺️
Unblocks #9224
