Give Command ownership over KeyChord #9543
Conversation
|
|
||
| if (keyChord) | ||
| { | ||
| command.KeyChordText(KeyChordSerialization::ToString(keyChord)); |
There was a problem hiding this comment.
Surprisingly, we still need something like this to support the following scenario:
{
"name": "Test...",
"commands": [
{ "name": "hello", "command": "copy" },
{ "name": "custom", "command": "openSettings", "keys": "ctrl+q" }
]
},
In this situation, "copy" still gets the "ctrl+c" key binding, but "openSettings" does not.
A fix will be included in the upcoming commit.
There was a problem hiding this comment.
For the record, the current implementation displays "hello" without a kbd, but "custom" with one. Exactly backwards. And ctrl+q does not actually execute the action.
There was a problem hiding this comment.
This is now fixed. As a part of ActionMap, it would be nice if we didn't have to do this. But I'm keeping an eye out to make sure this works as intended in the next PR.
For now, this is fine. ActionMap will probably get some tests to make sure we get the behavior we want.
| template<> | ||
| struct Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Control::KeyChord> | ||
| { | ||
| winrt::Microsoft::Terminal::Control::KeyChord FromJson(const Json::Value& json); | ||
| bool CanConvert(const Json::Value& json); | ||
| Json::Value ToJson(const winrt::Microsoft::Terminal::Control::KeyChord& val); | ||
| std::string TypeDescription() const; | ||
| }; | ||
|
|
There was a problem hiding this comment.
@DHowett Here's the beginning of a transition to adding more ConversionTraits. Hope this looks ok to you.
| @@ -39,7 +41,7 @@ the MIT License. See LICENSE in the project root for license information. --> | |||
| to make sure it takes the entire width of the line --> | |||
| <ListViewItem HorizontalContentAlignment="Stretch" | |||
| AutomationProperties.Name="{x:Bind Name, Mode=OneWay}" | |||
| AutomationProperties.AcceleratorKey="{x:Bind KeyChordText, Mode=OneWay}"> | |||
| AutomationProperties.AcceleratorKey="{x:Bind Keys, Mode=OneWay, Converter={StaticResource KeyChordToStringConverter}}"> | |||
There was a problem hiding this comment.
I originally wanted to make KeyChord IStringable and call it a day. But KeyChord is owned by TerminalControl, and I felt that it doesn't really make sense to add (de)serialization logic in there. If you disagree, let me know. The change would just require me to move KeyChordSerialization to TerminalControl.
|
Closing and merging code changes into #9621. |
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.
Transfers ownership of a
KeyChordtoCommand. This includes...KeyChordtoCommandkeysCommand::KeyChordTextCommand::KeyChordTextwhich caused...KeyChordto a stringKeyChordSerializationexperienced a minor refactor. I introduced theConversionTrait<KeyChord>there, and simply use the existingFromStringandToStringlogic to handle everything.References
#9428 - Spec
Validation Steps Performed
Checked the following...