Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bugfix: serialize iterable commands #10373

Merged
1 commit merged into from
Jun 10, 2021
Merged

Bugfix: serialize iterable commands #10373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
View file
Open in desktop
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
Open in desktop
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 @@ -296,6 +304,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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: why do we need to keep them separate from the nested commands?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping them separate does half of the work for this bug.

Combining them keeps the same buggy behavior from v1.8/main.

}

// repeat this for each of our parents
FAIL_FAST_IF(_parents.size() > 1);
for (const auto& parent : _parents)
Expand Down Expand Up @@ -336,6 +350,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
Open in desktop
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
Open in desktop
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
Open in desktop
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