Skip to content

Commit

Permalink
Convert most of our JSON deserializers to use type-based conversion (#…
Browse files Browse the repository at this point in the history 
…6590)

This pull request converts the following JSON deserializers to use the
new JSON deserializer pattern:

* Profile
* Command
* ColorScheme
* Action/Args
* GlobalSettings
* CascadiaSettingsSerialization

This is the completion of a long-term JSON refactoring that makes our
parser and deserializer more type-safe and robust. We're finally able to
get rid of all our manual enum conversion code and unify JSON conversion
around _types_ instead of around _keys_.

I've introduced another file filled with template specializations,
TerminalSettingsSerializationHelpers.h, which comprises a single unit
that holds all of the JSON deserializers (and eventually serializers)
for every type that comes from TerminalApp or TerminalSettings.

I've also moved some types out of Profile and GlobalAppSettings into a
new SettingsTypes.h to improve settings locality.

This does to some extent constitute a breaking change for already-broken
settings. Instead of parsing "successfully" (where invalid values are
null or 0 or unknown or unset), deserialization will now fail when
there's a type mismatch. Because of that, some tests had to be removed.

While I was on a refactoring spree, I removed a number of helpless
helpers, like GetWstringFromJson (which converted a u8 string to an
hstring to make a wstring out of its data pointer :|) and
_ConvertJsonToBool.

In the future, we can make the error types more robust and give them
position and type information such that a conformant application can
display rich error information ("line 3 column 3, I expected a string,
you gave me an integer").

Closes #2550.
  • Loading branch information
@DHowett
DHowett authored Jul 17, 2020
1 parent 7bc5de6 commit efb1fdd
Show file tree
Hide file tree
Showing 23 changed files with 877 additions and 1,738 deletions.
26 changes: 2 additions & 24 deletions src/cascadia/LocalTests_TerminalApp/CommandTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,8 @@ namespace TerminalAppLocalTests
{ "name": "command0", "command": { "action": "splitPane", "split": null } },
{ "name": "command1", "command": { "action": "splitPane", "split": "vertical" } },
{ "name": "command2", "command": { "action": "splitPane", "split": "horizontal" } },
{ "name": "command3", "command": { "action": "splitPane", "split": "none" } },
{ "name": "command4", "command": { "action": "splitPane" } },
{ "name": "command5", "command": { "action": "splitPane", "split": "auto" } },
{ "name": "command6", "command": { "action": "splitPane", "split": "foo" } }
{ "name": "command5", "command": { "action": "splitPane", "split": "auto" } }
])" };

const auto commands0Json = VerifyParseSucceeded(commands0String);
Expand All @@ -159,7 +157,7 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(0u, commands.size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(7u, commands.size());
VERIFY_ARE_EQUAL(5u, commands.size());

{
auto command = commands.at(L"command0");
Expand Down Expand Up @@ -191,16 +189,6 @@ namespace TerminalAppLocalTests
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Horizontal, realArgs.SplitStyle());
}
{
auto command = commands.at(L"command3");
VERIFY_IS_NOT_NULL(command);
VERIFY_IS_NOT_NULL(command.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action());
const auto& realArgs = command.Action().Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
{
auto command = commands.at(L"command4");
VERIFY_IS_NOT_NULL(command);
Expand All @@ -221,16 +209,6 @@ namespace TerminalAppLocalTests
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
{
auto command = commands.at(L"command6");
VERIFY_IS_NOT_NULL(command);
VERIFY_IS_NOT_NULL(command.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action());
const auto& realArgs = command.Action().Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
}
void CommandTests::TestResourceKeyName()
{
Expand Down
36 changes: 3 additions & 33 deletions src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,8 @@ namespace TerminalAppLocalTests
{ "keys": ["ctrl+c"], "command": { "action": "splitPane", "split": null } },
{ "keys": ["ctrl+d"], "command": { "action": "splitPane", "split": "vertical" } },
{ "keys": ["ctrl+e"], "command": { "action": "splitPane", "split": "horizontal" } },
{ "keys": ["ctrl+f"], "command": { "action": "splitPane", "split": "none" } },
{ "keys": ["ctrl+g"], "command": { "action": "splitPane" } },
{ "keys": ["ctrl+h"], "command": { "action": "splitPane", "split": "auto" } },
{ "keys": ["ctrl+i"], "command": { "action": "splitPane", "split": "foo" } }
{ "keys": ["ctrl+h"], "command": { "action": "splitPane", "split": "auto" } }
])" };

const auto bindings0Json = VerifyParseSucceeded(bindings0String);
Expand All @@ -335,7 +333,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(appKeyBindings);
VERIFY_ARE_EQUAL(0u, appKeyBindings->_keyShortcuts.size());
appKeyBindings->LayerJson(bindings0Json);
VERIFY_ARE_EQUAL(7u, appKeyBindings->_keyShortcuts.size());
VERIFY_ARE_EQUAL(5u, appKeyBindings->_keyShortcuts.size());

{
KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
Expand Down Expand Up @@ -364,15 +362,6 @@ namespace TerminalAppLocalTests
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Horizontal, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('F') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('G') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
Expand All @@ -391,23 +380,13 @@ namespace TerminalAppLocalTests
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('I') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Automatic, realArgs.SplitStyle());
}
}

void KeyBindingsTests::TestSetTabColorArgs()
{
const std::string bindings0String{ R"([
{ "keys": ["ctrl+c"], "command": { "action": "setTabColor", "color": null } },
{ "keys": ["ctrl+d"], "command": { "action": "setTabColor", "color": "#123456" } },
{ "keys": ["ctrl+e"], "command": { "action": "setTabColor", "color": "thisStringObviouslyWontWork" } },
{ "keys": ["ctrl+f"], "command": "setTabColor" },
])" };

Expand All @@ -417,7 +396,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(appKeyBindings);
VERIFY_ARE_EQUAL(0u, appKeyBindings->_keyShortcuts.size());
appKeyBindings->LayerJson(bindings0Json);
VERIFY_ARE_EQUAL(4u, appKeyBindings->_keyShortcuts.size());
VERIFY_ARE_EQUAL(3u, appKeyBindings->_keyShortcuts.size());

{
KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
Expand All @@ -439,15 +418,6 @@ namespace TerminalAppLocalTests
// Remember that COLORREFs are actually BBGGRR order, while the string is in #RRGGBB order
VERIFY_ARE_EQUAL(static_cast<uint32_t>(til::color(0x563412)), realArgs.TabColor().Value());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('E') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SetTabColorArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_IS_NULL(realArgs.TabColor());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('F') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
Expand Down
5 changes: 0 additions & 5 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1431,10 +1431,6 @@ namespace TerminalAppLocalTests
{
"name": "profile3",
"closeOnExit": null
},
{
"name": "profile4",
"closeOnExit": { "clearly": "not a string" }
}
]
})" };
Expand All @@ -1449,7 +1445,6 @@ namespace TerminalAppLocalTests

// Unknown modes parse as "Graceful"
VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[3].GetCloseOnExitMode());
VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[4].GetCloseOnExitMode());
}
void SettingsTests::TestCloseOnExitCompatibilityShim()
{
Expand Down
12 changes: 7 additions & 5 deletions src/cascadia/TerminalApp/ActionAndArgs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
#include "ActionArgs.h"
#include "ActionAndArgs.h"
#include "ActionAndArgs.g.cpp"

#include "JsonUtils.h"

#include <LibraryResources.h>

static constexpr std::string_view CopyTextKey{ "copy" };
Expand Down Expand Up @@ -44,6 +47,8 @@ static constexpr std::string_view UnboundKey{ "unbound" };

namespace winrt::TerminalApp::implementation
{
using namespace ::TerminalApp;

// Specifically use a map here over an unordered_map. We want to be able to
// iterate over these entries in-order when we're serializing the keybindings.
// HERE BE DRAGONS:
Expand Down Expand Up @@ -183,11 +188,9 @@ namespace winrt::TerminalApp::implementation
}
else if (json.isObject())
{
const auto actionVal = json[JsonKey(ActionKey)];
if (actionVal.isString())
if (const auto actionString{ JsonUtils::GetValueForKey<std::optional<std::string>>(json, ActionKey) })
{
auto actionString = actionVal.asString();
action = GetActionFromString(actionString);
action = GetActionFromString(*actionString);
argsVal = json;
}
}
Expand Down Expand Up @@ -281,5 +284,4 @@ namespace winrt::TerminalApp::implementation
const auto found = GeneratedActionNames.find(_Action);
return found != GeneratedActionNames.end() ? found->second : L"";
}

}
Loading

0 comments on commit efb1fdd

Please sign in to comment.