From 9294ecc8e52a61bb0d47b56a0a5e3c42734427ba Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 10 Jun 2021 14:25:27 -0400 Subject: [PATCH] Bugfix: serialize iterable commands (#10373) ## 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. --- .../SerializationTests.cpp | 23 +++++++++++ .../TerminalSettingsModel/ActionMap.cpp | 33 +++++++++++++--- .../TerminalSettingsModel/ActionMap.h | 3 +- .../ActionMapSerialization.cpp | 38 +++++++++++-------- .../TerminalSettingsModel/Command.cpp | 6 +-- 5 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp index 97251b8b2..164ac0bb8 100644 --- a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp @@ -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" } } ])" }; @@ -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...", @@ -278,6 +286,7 @@ namespace SettingsModelLocalTests } ])" }; + // iterable command const std::string actionsString9A{ R"([ { "name": "New tab", @@ -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" } ])" }; @@ -365,6 +387,7 @@ namespace SettingsModelLocalTests RoundtripTest(actionsString9A); RoundtripTest(actionsString9B); RoundtripTest(actionsString9C); + RoundtripTest(actionsString9D); Log::Comment(L"unbound command"); RoundtripTest(actionsString10); diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 433535dc5..a9ab094e7 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -29,7 +29,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } ActionMap::ActionMap() : - _NestedCommands{ single_threaded_map() } + _NestedCommands{ single_threaded_map() }, + _IterableCommands{ single_threaded_vector() } { } @@ -87,7 +88,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { // populate _NameMapCache std::unordered_map nameMap{}; - _PopulateNameMapWithNestedCommands(nameMap); + _PopulateNameMapWithSpecialCommands(nameMap); _PopulateNameMapWithStandardCommands(nameMap); _NameMapCache = single_threaded_map(std::move(nameMap)); @@ -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& nameMap) const + void ActionMap::_PopulateNameMapWithSpecialCommands(std::unordered_map& 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. @@ -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: @@ -296,6 +304,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation actionMap->_NestedCommands.Insert(name, *(get_self(cmd)->Copy())); } + // copy _IterableCommands + for (const auto& cmd : _IterableCommands) + { + actionMap->_IterableCommands.Append(*(get_self(cmd)->Copy())); + } + // repeat this for each of our parents FAIL_FAST_IF(_parents.size() > 1); for (const auto& parent : _parents) @@ -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. diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index e33638cff..3435d120c 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -81,7 +81,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::optional _GetActionByID(const InternalActionID actionID) const; std::optional _GetActionByKeyChordInternal(Control::KeyChord const& keys) const; - void _PopulateNameMapWithNestedCommands(std::unordered_map& nameMap) const; + void _PopulateNameMapWithSpecialCommands(std::unordered_map& nameMap) const; void _PopulateNameMapWithStandardCommands(std::unordered_map& nameMap) const; void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map& keyBindingsMap, std::unordered_set& unboundKeys) const; std::vector _GetCumulativeActions() const noexcept; @@ -94,6 +94,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Windows::Foundation::Collections::IMap _GlobalHotkeysCache{ nullptr }; Windows::Foundation::Collections::IMap _KeyBindingMapCache{ nullptr }; Windows::Foundation::Collections::IMap _NestedCommands{ nullptr }; + Windows::Foundation::Collections::IVector _IterableCommands{ nullptr }; std::unordered_map _KeyMap; std::unordered_map _ActionMap; diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index 1358c7865..5956bf757 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -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(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(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; } diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index ee560b78d..dbdfd6e5c 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -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);