Skip to content

Commit

Permalink
leonard comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@PankajBhojwani
PankajBhojwani committed May 20, 2024
1 parent 14d83b5 commit 6c6dd46
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 73 deletions.
57 changes: 21 additions & 36 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void ActionMap::_FinalizeInheritance()
{
// first, gather the inbox actions from the relevant parent
std::unordered_map<InternalActionID, Model::Command> InboxActions;
std::unordered_map<InternalActionID, Model::Command> inboxActions;
winrt::com_ptr<implementation::ActionMap> foundParent{ nullptr };
for (const auto parent : _parents)
for (const auto& parent : _parents)
{
for (const auto [_, cmd] : parent->_ActionMap)
for (const auto& [_, cmd] : parent->_ActionMap)
{
if (cmd.Origin() != OriginTag::InBox)
{
// only one parent contains all the inbox actions and that parent contains only inbox actions,
// so if we found a non-inbox action we can just skip to the next parent
break;
continue;
}
foundParent = parent;
break;
Expand All @@ -85,9 +85,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

if (foundParent)
{
for (const auto [_, cmd] : foundParent->_ActionMap)
for (const auto& [_, cmd] : foundParent->_ActionMap)
{
InboxActions.emplace(Hash(cmd.ActionAndArgs()), cmd);
inboxActions.emplace(Hash(cmd.ActionAndArgs()), cmd);
}
}

Expand All @@ -105,9 +105,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
if (userCmdImpl->IdWasGenerated() && !userCmdImpl->HasName() && userCmd.IconPath().empty())
{
const auto userActionHash = Hash(userCmd.ActionAndArgs());
if (const auto inboxCmd = InboxActions.find(userActionHash); inboxCmd != InboxActions.end())
if (const auto inboxCmd = inboxActions.find(userActionHash); inboxCmd != inboxActions.end())
{
for (auto [key, cmdID] : _KeyMap)
for (const auto& [key, cmdID] : _KeyMap)
{
// for any of our keys that point to the user action, point them to the inbox action instead
if (cmdID == userID)
Expand All @@ -126,10 +126,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}

// now, remove the commands with the IDs we found
for (const auto id : IdsToRemove)
{
_ActionMap.erase(id);
}
std::erase_if(_ActionMap, [&IdsToRemove](const auto& pair) { return IdsToRemove.contains(pair.first); });
}

bool ActionMap::FixUpsAppliedDuringLoad() const
Expand Down Expand Up @@ -158,7 +155,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return cmd;
}

for (const auto parent : _parents)
for (const auto& parent : _parents)
{
if (const auto inheritedCmd = parent->_GetActionByID(actionID))
{
Expand Down Expand Up @@ -311,7 +308,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// there might be a collision here, where there could be 2 different commands with the same name
// in this case, prioritize the user's action
// TODO GH #17166: we should no longer use Command.Name to identify commands anywhere
if (nameMap.find(name) == nameMap.end() || cmd.Origin() == OriginTag::User)
if (!nameMap.contains(name) || cmd.Origin() == OriginTag::User)
{
// either a command with this name does not exist, or this is a user-defined command with a name
// in either case, update the name map with the command (if this is a user-defined command with
Expand All @@ -329,13 +326,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
for (const auto& [keys, cmdID] : _KeyMap)
{
if (keyBindingsMap.find(keys) == keyBindingsMap.end())
if (!keyBindingsMap.contains(keys))
{
keyBindingsMap.emplace(keys, cmdID);
}
}

for (const auto parent : _parents)
for (const auto& parent : _parents)
{
parent->_PopulateCumulativeKeyMap(keyBindingsMap);
}
Expand All @@ -348,13 +345,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
for (const auto& [cmdID, cmd] : _ActionMap)
{
if (actionMap.find(cmdID) == actionMap.end())
if (!actionMap.contains(cmdID))
{
actionMap.emplace(cmdID, cmd);
}
}

for (const auto parent : _parents)
for (const auto& parent : _parents)
{
parent->_PopulateCumulativeActionMap(actionMap);
}
Expand Down Expand Up @@ -388,7 +385,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
_PopulateCumulativeKeyMap(accumulatedKeybindingsMap);
_PopulateCumulativeActionMap(accumulatedActionsMap);

for (const auto [keys, cmdID] : accumulatedKeybindingsMap)
for (const auto& [keys, cmdID] : accumulatedKeybindingsMap)
{
if (const auto idCmdPair = accumulatedActionsMap.find(cmdID); idCmdPair != accumulatedActionsMap.end())
{
Expand Down Expand Up @@ -576,14 +573,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// i.e. they provided an ID for a null command (which they really shouldn't, there's no purpose)
// in this case, we do _not_ want to use the id they provided, we want to use an empty id
// (empty id in the _KeyMap indicates the keychord was explicitly unbound)
if (cmd.ActionAndArgs().Action() == ShortcutAction::Invalid)
{
_KeyMap.insert_or_assign(keys, L"");
}
else
{
_KeyMap.insert_or_assign(keys, cmd.ID());
}
const auto action = cmd.ActionAndArgs().Action();
const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();
_KeyMap.insert_or_assign(keys, id);
}

// Method Description:
Expand Down Expand Up @@ -626,15 +618,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
if (const auto keyIDPair = _KeyMap.find(keys); keyIDPair != _KeyMap.end())
{
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"";
}
// the keychord is defined in this layer, return the ID (ID be empty, in which case this key is explicitly unbound)
return keyIDPair->second;
}

// search through our parents
Expand Down
71 changes: 34 additions & 37 deletions src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
Json::Value keybindingsList{ Json::ValueType::arrayValue };

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);
};

// Serialize all standard keybinding objects in the current layer
for (const auto& [keys, cmdID] : _KeyMap)
{
toJson(keys, cmdID);
Json::Value keyIDPair{ Json::ValueType::objectValue };
JsonUtils::SetValueForKey(keyIDPair, KeysKey, keys);
JsonUtils::SetValueForKey(keyIDPair, IDKey, cmdID);
keybindingsList.append(keyIDPair);
}

return keybindingsList;
Expand All @@ -151,40 +147,41 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
if (keysJson.isArray() && keysJson.size() > 1)
{
warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
return;
}
else

Control::KeyChord keys{ nullptr };
winrt::hstring idJson;
if (!JsonUtils::GetValueForKey(json, KeysKey, keys))
{
Control::KeyChord keys{ nullptr };
winrt::hstring idJson;
if (JsonUtils::GetValueForKey(json, KeysKey, keys))
{
// if these keys are already bound to some command,
// we need to update that command to erase these keys as we are about to overwrite them
if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)
{
const auto foundCommandImpl{ get_self<implementation::Command>(*foundCommand) };
foundCommandImpl->EraseKey(keys);
}
return;
}

// if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine
JsonUtils::GetValueForKey(json, IDKey, idJson);
// if these keys are already bound to some command,
// we need to update that command to erase these keys as we are about to overwrite them
if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)
{
const auto foundCommandImpl{ get_self<implementation::Command>(*foundCommand) };
foundCommandImpl->EraseKey(keys);
}

// any existing keybinding with the same keychord in this layer will get overwritten
_KeyMap.insert_or_assign(keys, idJson);
// if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine
JsonUtils::GetValueForKey(json, IDKey, idJson);

// make sure the command registers these keys
if (!idJson.empty())
{
// TODO GH#17160
// if the command with this id is only going to appear later during settings load
// then this will return null, meaning that the command created later on will not register this keybinding
// the keybinding will still work fine within the app, its just that the Command object itself won't know about this key mapping
// we are going to move away from Command needing to know its key mappings in a followup, so this shouldn't matter for very long
if (const auto cmd = _GetActionByID(idJson))
{
cmd.RegisterKey(keys);
}
}
// any existing keybinding with the same keychord in this layer will get overwritten
_KeyMap.insert_or_assign(keys, idJson);

// make sure the command registers these keys
if (!idJson.empty())
{
// TODO GH#17160
// if the command with this id is only going to appear later during settings load
// then this will return null, meaning that the command created later on will not register this keybinding
// the keybinding will still work fine within the app, its just that the Command object itself won't know about this key mapping
// we are going to move away from Command needing to know its key mappings in a followup, so this shouldn't matter for very long
if (const auto cmd = _GetActionByID(idJson))
{
cmd.RegisterKey(keys);
}
}
return;
Expand Down

0 comments on commit 6c6dd46

Please sign in to comment.