maxmustermann
/
terminal
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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.
This commit is contained in:
Carlos Zamora 2021-06-10 14:25:27 -04:00 committed by GitHub
parent 8157605058
commit 9294ecc8e5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 77 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
View File

@ -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<implementation::ActionMap>(actionsString9A);
RoundtripTest<implementation::ActionMap>(actionsString9B);
RoundtripTest<implementation::ActionMap>(actionsString9C);
RoundtripTest<implementation::ActionMap>(actionsString9D);
Log::Comment(L"unbound command");
RoundtripTest<implementation::ActionMap>(actionsString10);

33
src/cascadia/TerminalSettingsModel/ActionMap.cpp
View File

@ -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>() }
{
}
@ -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));
@ -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.
@ -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<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)
@ -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.

3
src/cascadia/TerminalSettingsModel/ActionMap.h
View File

@ -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;
@ -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;

38
src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View File

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

6
src/cascadia/TerminalSettingsModel/Command.cpp
View File

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