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.
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
Refactor ActionMap and Command to use ActionIDs #17162
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, and 1 bug.
I'm not sure I understood how all the changes fit together in the grand scheme of things. I'm not so deep into the ActionMap code. 🙈
auto toJson = [&keybindingsList](const KeyChord kc, const winrt::hstring cmdID) { | ||
Json::Value keyIDPair{ Json::ValueType::objectValue }; | ||
JsonUtils::SetValueForKey(keyIDPair, KeysKey, kc); | ||
JsonUtils::SetValueForKey(keyIDPair, IDKey, cmdID); | ||
keybindingsList.append(keyIDPair); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lambda seems unnecessary.
{ | ||
warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord); | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early-return and unnest the rest?
{ | ||
Control::KeyChord keys{ nullptr }; | ||
winrt::hstring idJson; | ||
if (JsonUtils::GetValueForKey(json, KeysKey, keys)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
_KeyMap.insert_or_assign(keys, idJson); | ||
|
||
// make sure the command registers these keys | ||
if (!idJson.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
else | ||
{ | ||
// this is not a command block, so it is a keybinding block | ||
_AddKeyBindingHelper(jsonBlock, warnings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you invert the if condition, you can flip the branches and use continue
here. It would still make sense that way IMO, because it would read "if it has no commands and no actions, then add an error and continue".
const auto parentActions{ parent->_GetCumulativeActions() }; | ||
cumulativeActions.reserve(cumulativeActions.size() + parentActions.size()); | ||
cumulativeActions.insert(cumulativeActions.end(), parentActions.begin(), parentActions.end()); | ||
if (actionMap.find(cmdID) == actionMap.end()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here)
} | ||
|
||
return cumulativeActions; | ||
for (const auto parent : _parents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here)
|
||
for (const auto& [keys, cmd] : keyBindingsMap) | ||
for (const auto [keys, cmdID] : accumulatedKeybindingsMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here)
if (cmd.ActionAndArgs().Action() == ShortcutAction::Invalid) | ||
{ | ||
// Update inner Command with new key chord | ||
auto oldCmdImpl{ get_self<Command>(oldCmd) }; | ||
oldCmdImpl->RegisterKey(keys); | ||
_KeyMap.insert_or_assign(keys, L""); | ||
} | ||
|
||
// Additive operation: | ||
// Register the new key chord with maskingCmd (an existing _maskingAction entry) | ||
// Example: | ||
// parent: { "command": "copy", "keys": "ctrl+c" } --> register "ctrl+c" to parent._ActionMap (different branch in a different layer) | ||
// current: { "command": "copy", "keys": "ctrl+shift+c" } --> also register "ctrl+shift+c" to the same Command (maskingCmd) | ||
if (maskingCmd) | ||
else | ||
{ | ||
// Update inner Command with new key chord | ||
auto maskingCmdImpl{ get_self<Command>(maskingCmd) }; | ||
maskingCmdImpl->RegisterKey(keys); | ||
_KeyMap.insert_or_assign(keys, cmd.ID()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something like
hstring id;
if (cmd.ActionAndArgs().Action() == ShortcutAction::Invalid)
{
id = cmd.ID();
}
_KeyMap.insert_or_assign(keys, id);
or
const auto action = cmd.ActionAndArgs().Action();
const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();
_KeyMap.insert_or_assign(keys, id);
I'm not sure I understand why we want to store an empty string in the map for invalid actions though. Shouldn't we want to remove the key entirely?
if (const auto cmdID = keyIDPair->second; !cmdID.empty()) | ||
{ | ||
return cmdID; | ||
} | ||
else | ||
{ | ||
// the keychord is defined in this layer, but points to an empty string - explicitly unbound | ||
return L""; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's basically the same as just return keyIDPair->second
.
Summary of the Pull Request
As outlined in #16816, refactor
ActionMap
to 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