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

Convert most of our JSON deserializers to use type-based conversion #6590

Merged
27 commits merged into from
Jul 17, 2020
Merged

Convert most of our JSON deserializers to use type-based conversion #6590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
112b42d
[BRIDGE] JsonUtils->JsonUtilsNew
DHowett Jun 3, 2020
736debe
ColorScheme: Convert to JsonUtilsNew
DHowett May 10, 2020
f4cb75e
Profile: Convert to JsonUtilsNew
DHowett May 10, 2020
7aeed1c
GlobalAppSettings: Convert to JsonUtilsNew
DHowett May 11, 2020
1c02bf2
CSS: Convert to JsonUtilsNew
DHowett May 12, 2020
372cb90
GAS: move to macro
DHowett Jun 4, 2020
8f33d71
Profile: move to mapper
DHowett Jun 4, 2020
a55198d
remove GetWstringFromJson & utils.cpp
DHowett Jun 19, 2020
163f0f2
Some tests are now invalid
DHowett Jun 19, 2020
32ee8df
GAS: clean up
DHowett Jun 19, 2020
73b18f0
Action/Args: Convert
DHowett Jun 19, 2020
785f7c0
profile: convert fontweight, move const strings
DHowett Jun 19, 2020
3730707
Command: update to JU-NEW
DHowett Jul 12, 2020
16ab0ae
ProfileConversions: merge constexpr strings w/ map
DHowett Jul 12, 2020
627ede2
Add TermSettingsSerHelpers, move all settings traits into it
DHowett Jul 14, 2020
05b31a6
remove dead line
DHowett Jul 14, 2020
5d1fd5a
code format
DHowett Jul 14, 2020
5dbe104
move strings/actargs helpers
DHowett Jul 14, 2020
d040df4
fode cormat
DHowett Jul 14, 2020
6f8deaf
Merge remote-tracking branch 'origin/master' into dev/duhowett/json-2…
DHowett Jul 14, 2020
59e54d1
clean
DHowett Jul 14, 2020
9801aa6
git is mad, push an empty commit
DHowett Jul 14, 2020
9b1e7a3
Merge remote-tracking branch 'origin/master' into dev/duhowett/json-2…
DHowett Jul 15, 2020
a23bc51
Fixup after merge
DHowett Jul 15, 2020
f18374e
rename JU-NEW, fix an Oops in TargetType/ValueType
DHowett Jul 15, 2020
e6c229a
update test for rename
DHowett Jul 15, 2020
a8841d4
remove more tests that explicitly parsed bad values
DHowett Jul 15, 2020
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
26 changes: 2 additions & 24 deletions src/cascadia/LocalTests_TerminalApp/CommandTests.cpp
View file
Open in desktop
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
Open in desktop
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" } }
DHowett marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Should we add back a test case that ensures a warning was returned for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, this causes a whole settings parsing failure... but the specific case of deserializing an unknown enum member is actually handled in the JsonUtilsTests. Should we have one specifically for a "known type of flag"?

{ "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
Open in desktop
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
Open in desktop
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