Refactor ActionMap and Command to use ActionIDs #17162
Conversation
| { | ||
| if (cmd.ActionAndArgs().Action() != ShortcutAction::Invalid) | ||
| // Only populate AvailableActions with actions that haven't been visited already. | ||
| const auto actionID = Hash(cmd.ActionAndArgs()); |
There was a problem hiding this comment.
Unfortunately we still have to use InternalActionID to populate the available action dropdown in the SUI since we don't have a Terminal.<> id for every configuration of each shortcut action and its args
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
|
| @@ -1023,7 +1023,8 @@ namespace winrt::TerminalApp::implementation | |||
| // NewTab(ProfileIndex=N) action | |||
| NewTerminalArgs newTerminalArgs{ profileIndex }; | |||
| NewTabArgs newTabArgs{ newTerminalArgs }; | |||
| auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(ShortcutAction::NewTab, newTabArgs) }; | |||
| const auto id = fmt::format(FMT_COMPILE(L"Terminal.OpenNewTabProfile{}"), profileIndex); | |||
| const auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(id) }; | |||
There was a problem hiding this comment.
There 100% is a issue right now for "I rebound keys for new tab profile N, but the dropdown still has the old ones" and I'm guessing this will solve that (because people can rebind the key for the literal Terminal.OpenNewTabProfile5 rather than just making a new Command for it)
There was a problem hiding this comment.
Yep this fixes that! Also fixes it in the cmd palette
| { "command": "paste", "keys": "ctrl+v" }, | ||
| { "command": "find", "keys": "ctrl+shift+f" }, | ||
| { "command": { "action": "splitPane", "split": "auto", "splitMode": "duplicate" }, "keys": "alt+shift+d" } | ||
| { "id": "Terminal.CopySelectedText", "keys": "ctrl+c" }, |
There was a problem hiding this comment.
Okay so, if I want to
- Remove the ctrl+shift+c keybinding set in the defaults
- AND add a keybinding to alt+c
how do I do that now?
Does this add a ctrl+c keybinding to Terminal.CopySelectedText, or replace the original ctrl+shift+c?
There was a problem hiding this comment.
Does this add a ctrl+c keybinding to Terminal.CopySelectedText, or replace the original ctrl+shift+c?
It only adds!
Remove the ctrl+shift+c keybinding set in the defaults
Then you add {"id": null, "keys": "ctrl+shift+c"} into the keybindings array
AND add a keybinding to alt+c
Add {"id": "<id of the command>", "keys": "alt+c"} into keybindings
| { "command": "copy", "keys": ["ctrl+c"] }, | ||
| { "command": { "action": "copy", "singleLine": false }, "keys": ["ctrl+shift+c"] }, | ||
| { "command": { "action": "copy", "singleLine": true }, "keys": ["alt+shift+c"] }, | ||
| { "command": "copy", "id": "Test.CopyNoArgs", "keys": ["ctrl+c"] }, |
There was a problem hiding this comment.
for my own notes: the commands in this test needed id's because the JSON was getting parsed as origin::none? if it was origin::user, they wouldn't need id's, right?
There was a problem hiding this comment.
Yes, since they are Origin::None we will not generate ids for them and so the ActionMap will refuse to add them.
Summary of the Pull Request
As outlined in #16816, refactor
ActionMapto use the new action IDs added in #16904Detailed description of the pull request
See the spec!
Validation steps performed
References and Relevant Issues
#16816
PR Checklist