Skip to content

Commit

Permalink
Bugfix: serialize iterable commands (#10373)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
Fixes a bug where top-level iterable commands were not serialized.

## PR Checklist
* [X] Closes #10365
* [X] Tests added/passed

## Detailed Description of the Pull Request / Additional comments
- `Command::ToJson`:
   - iterable commands deserve the same treatment as nested commands
- `ActionMap`:
   - Similar to how we store nested commands, iterable commands need to be handled separately from standard commands. Then, when generating the name map, we make sure we export the iterable commands at the same time we export the nested commands.

(cherry picked from commit 9294ecc)
  • Loading branch information
@carlos-zamora @DHowett
carlos-zamora authored and DHowett committed Jul 7, 2021
1 parent 3fd8ebf commit 658e6e6
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 26 deletions.
23 changes: 23 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,12 @@ namespace SettingsModelLocalTests

void SerializationTests::Actions()
{
// simple command
const std::string actionsString1{ R"([
{ "command": "paste" }
])" };

// complex command
const std::string actionsString2A{ R"([
{ "command": { "action": "setTabColor" } }
])" };
Expand All @@ -244,29 +246,35 @@ namespace SettingsModelLocalTests
{ "command": { "action": "copy", "singleLine": true, "copyFormatting": "html" } }
])" };

// simple command with key chords
const std::string actionsString3{ R"([
{ "command": "toggleAlwaysOnTop", "keys": "ctrl+a" },
{ "command": "toggleAlwaysOnTop", "keys": "ctrl+b" }
])" };

// complex command with key chords
const std::string actionsString4{ R"([
{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+c" },
{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+d" }
])" };

// command with name and icon and multiple key chords
const std::string actionsString5{ R"([
{ "icon": "image.png", "name": "Scroll To Top Name", "command": "scrollToTop", "keys": "ctrl+e" },
{ "command": "scrollToTop", "keys": "ctrl+f" }
])" };

// complex command with new terminal args
const std::string actionsString6{ R"([
{ "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+g" },
])" };

// complex command with meaningful null arg
const std::string actionsString7{ R"([
{ "command": { "action": "renameWindow", "name": null }, "keys": "ctrl+h" }
])" };

// nested command
const std::string actionsString8{ R"([
{
"name": "Change font size...",
Expand All @@ -278,6 +286,7 @@ namespace SettingsModelLocalTests
}
])" };

// iterable command
const std::string actionsString9A{ R"([
{
"name": "New tab",
Expand Down Expand Up @@ -330,7 +339,20 @@ namespace SettingsModelLocalTests
"name": "Send Input (Evil) ..."
}
])"" };
const std::string actionsString9D{ R""([
{
"command":
{
"action": "newTab",
"profile": "${profile.name}"
},
"icon": "${profile.icon}",
"iterateOn": "profiles",
"name": "${profile.name}: New tab"
}
])"" };

// unbound command
const std::string actionsString10{ R"([
{ "command": "unbound", "keys": "ctrl+c" }
])" };
Expand Down Expand Up @@ -365,6 +387,7 @@ namespace SettingsModelLocalTests
RoundtripTest<implementation::ActionMap>(actionsString9A);
RoundtripTest<implementation::ActionMap>(actionsString9B);
RoundtripTest<implementation::ActionMap>(actionsString9C);
RoundtripTest<implementation::ActionMap>(actionsString9D);

Log::Comment(L"unbound command");
RoundtripTest<implementation::ActionMap>(actionsString10);
Expand Down
33 changes: 27 additions & 6 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}

ActionMap::ActionMap() :
_NestedCommands{ single_threaded_map<hstring, Model::Command>() }
_NestedCommands{ single_threaded_map<hstring, Model::Command>() },
_IterableCommands{ single_threaded_vector<Model::Command>() }
{
}

Expand Down Expand Up @@ -87,7 +88,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
// populate _NameMapCache
std::unordered_map<hstring, Model::Command> nameMap{};
_PopulateNameMapWithNestedCommands(nameMap);
_PopulateNameMapWithSpecialCommands(nameMap);
_PopulateNameMapWithStandardCommands(nameMap);

_NameMapCache = single_threaded_map<hstring, Model::Command>(std::move(nameMap));
Expand All @@ -96,18 +97,19 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}

// Method Description:
// - Populates the provided nameMap with all of our nested commands and our parents nested commands
// - Performs a top-down approach by going to the root first, then recursively adding the nested commands layer-by-layer
// - Populates the provided nameMap with all of our special commands and our parent's special commands.
// - Special commands include nested and iterable commands.
// - Performs a top-down approach by going to the root first, then recursively adding the nested commands layer-by-layer.
// Arguments:
// - nameMap: the nameMap we're populating. This maps the name (hstring) of a command to the command itself.
void ActionMap::_PopulateNameMapWithNestedCommands(std::unordered_map<hstring, Model::Command>& nameMap) const
void ActionMap::_PopulateNameMapWithSpecialCommands(std::unordered_map<hstring, Model::Command>& nameMap) const
{
// Update NameMap with our parents.
// Starting with this means we're doing a top-down approach.
FAIL_FAST_IF(_parents.size() > 1);
for (const auto& parent : _parents)
{
parent->_PopulateNameMapWithNestedCommands(nameMap);
parent->_PopulateNameMapWithSpecialCommands(nameMap);
}

// Add NestedCommands to NameMap _after_ we handle our parents.
Expand All @@ -125,6 +127,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
nameMap.erase(name);
}
}

// Add IterableCommands to NameMap
for (const auto& cmd : _IterableCommands)
{
nameMap.insert_or_assign(cmd.Name(), cmd);
}
}

// Method Description:
Expand Down Expand Up @@ -291,6 +299,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
actionMap->_NestedCommands.Insert(name, *(get_self<Command>(cmd)->Copy()));
}

// copy _IterableCommands
for (const auto& cmd : _IterableCommands)
{
actionMap->_IterableCommands.Append(*(get_self<Command>(cmd)->Copy()));
}

// repeat this for each of our parents
FAIL_FAST_IF(_parents.size() > 1);
for (const auto& parent : _parents)
Expand Down Expand Up @@ -331,6 +345,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return;
}

// Handle iterable commands
if (cmdImpl->IterateOn() != ExpandCommandType::None)
{
_IterableCommands.Append(cmd);
return;
}

// General Case:
// Add the new command to the KeyMap.
// This map directs you to an entry in the ActionMap.
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsModel/ActionMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
std::optional<Model::Command> _GetActionByID(const InternalActionID actionID) const;
std::optional<Model::Command> _GetActionByKeyChordInternal(Control::KeyChord const& keys) const;

void _PopulateNameMapWithNestedCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
void _PopulateNameMapWithSpecialCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
void _PopulateNameMapWithStandardCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const;
std::vector<Model::Command> _GetCumulativeActions() const noexcept;
Expand All @@ -94,6 +94,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _GlobalHotkeysCache{ nullptr };
Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _KeyBindingMapCache{ nullptr };
Windows::Foundation::Collections::IMap<hstring, Model::Command> _NestedCommands{ nullptr };
Windows::Foundation::Collections::IVector<Model::Command> _IterableCommands{ nullptr };
std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;
std::unordered_map<InternalActionID, Model::Command> _ActionMap;

Expand Down
38 changes: 22 additions & 16 deletions src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,34 +60,40 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
Json::Value actionList{ Json::ValueType::arrayValue };

// Serialize all standard Command objects in the current layer
for (const auto& [_, cmd] : _ActionMap)
{
// Command serializes to an array of JSON objects.
// This is because a Command may have multiple key chords associated with it.
// The name and icon are only serialized in the first object.
// Example:
// { "name": "Custom Copy", "command": "copy", "keys": "ctrl+c" }
// { "command": "copy", "keys": "ctrl+shift+c" }
// { "command": "copy", "keys": "ctrl+ins" }
// Command serializes to an array of JSON objects.
// This is because a Command may have multiple key chords associated with it.
// The name and icon are only serialized in the first object.
// Example:
// { "name": "Custom Copy", "command": "copy", "keys": "ctrl+c" }
// { "command": "copy", "keys": "ctrl+shift+c" }
// { "command": "copy", "keys": "ctrl+ins" }
auto toJson = [&actionList](const Model::Command& cmd) {
const auto cmdImpl{ winrt::get_self<implementation::Command>(cmd) };
const auto& cmdJsonArray{ cmdImpl->ToJson() };
for (const auto& cmdJson : cmdJsonArray)
{
actionList.append(cmdJson);
}
};

// Serialize all standard Command objects in the current layer
for (const auto& [_, cmd] : _ActionMap)
{
toJson(cmd);
}

// Serialize all nested Command objects added in the current layer
for (const auto& [_, cmd] : _NestedCommands)
{
const auto cmdImpl{ winrt::get_self<implementation::Command>(cmd) };
const auto& cmdJsonArray{ cmdImpl->ToJson() };
for (const auto& cmdJson : cmdJsonArray)
{
actionList.append(cmdJson);
}
toJson(cmd);
}

// Serialize all iterable Command objects added in the current layer
for (const auto& cmd : _IterableCommands)
{
toJson(cmd);
}

return actionList;
}

Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsModel/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
Json::Value cmdList{ Json::ValueType::arrayValue };

if (_nestedCommand)
if (_nestedCommand || _IterateOn != ExpandCommandType::None)
{
// handle nested command
// For nested commands, we can trust _originalJson to be correct.
// handle special commands
// For these, we can trust _originalJson to be correct.
// In fact, we _need_ to use it here because we don't actually deserialize `iterateOn`
// until we expand the command.
cmdList.append(_originalJson);
Expand Down

0 comments on commit 658e6e6

Please sign in to comment.