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

Layer the globals globals on top of the root globals #3031

Merged
merged 2 commits into from
Oct 4, 2019
Merged

Layer the globals globals on top of the root globals #3031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cf5b67
Layer the globals globals on top of the root globals
zadjii-msft Oct 2, 2019
9934049
add more test cases for @carlos-zamora
zadjii-msft Oct 4, 2019
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
112 changes: 112 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ namespace TerminalAppLocalTests
TEST_METHOD(TestExplodingNameOnlyProfiles);
TEST_METHOD(TestHideAllProfiles);

TEST_METHOD(TestLayerGlobalsOnRoot);

TEST_CLASS_SETUP(ClassSetup)
{
InitializeJsonReader();
Expand Down Expand Up @@ -1191,4 +1193,114 @@ namespace TerminalAppLocalTests
VERIFY_IS_TRUE(caughtExpectedException);
}
}

void SettingsTests::TestLayerGlobalsOnRoot()
Copy link
Member

@carlos-zamora carlos-zamora Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What if the user has multiple "globals"? Like the following:

"globals":{
   "defaultProfile": <val>
 },
"defaultProfile": <val>,
"globals":{
   "defaultProfile": <val>
}

This might be impacted by the other thread I started...

Copy link
Contributor

Choose a reason for hiding this comment

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

this is just gonna be invalid

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'd kinda just prefer to remove the "globals" key in general. Then we'd get rid of all of these 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

me too at this point. However, we haven't ever gotten to the point where we can safely say "we're breaking your settings now" unfortunately. Plus there's that C# tool out there for customizing our settings, and IIRC that tool uses the "globals" object :(

{
// Test for microsoft/terminal#2906. We added the ability for the root
// to be used as the globals object in #2515. However, if you have a
// globals object, then the settings in the root would get ignored.
// This test ensures that settings from a child "globals" element
// _layer_ on top of root properties, and they don't cause the root
// properties to be totally ignored.

const std::string settings0String{ R"(
{
"globals": {
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"initialRows": 123
}
})" };
const std::string settings1String{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"initialRows": 234
})" };
const std::string settings2String{ R"(
{
"defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"initialRows": 345,
"globals": {
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
// initialRows should not be cleared here
}
})" };
const std::string settings3String{ R"(
Copy link
Member

Choose a reason for hiding this comment

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

Could you add another test:

        const std::string settings4String{ R"(
        {
            "initialRows": 345,
            "globals": {
                "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
                // initialRows should not be cleared here
            },
            "defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
        })" };

I think as of now, processing this will make defaultProfile {6239a42c-1111-49a3-80bd-e8fdd045185c} instead of {6239a42c-2222-49a3-80bd-e8fdd045185c}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but I think that's the expected behavior with this change unfortunately. Once we've parsed the json, it doesn't retain any of its original ordering any longer. I'm not sure how we'd be able to get the 2222 guid to be the default in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I think that's the expected behavior with this change unfortunately. Once we've parsed the json, it doesn't retain any of its original ordering any longer. I'm not sure how we'd be able to get the 2222 guid to be the default in that case.

Think it might be worth creating an issue then and just leaving it for later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, i see: file level ordering. Interesting.

...

we can check getOffsetStart and getOffsetLimit 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

gosh Dustin why'd you have to suggest that

That's certainly an option, but that's a lot harder. We'd have to do it on a peculiar property-by-property basis. Layering wouldn't just be "do I have this property? great, let's parse it". It's now "for the globals, do I have this property? was its getOffsetStart greater than the last time I layered this property?"

That takes it from a little change to a much, MUCH bigger change

{
"defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"globals": {
"initialRows": 456
// defaultProfile should not be cleared here
}
})" };
const std::string settings4String{ R"(
{
"defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"globals": {
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
},
"defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}"
})" };
const std::string settings5String{ R"(
{
"globals": {
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
},
"defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"globals": {
"defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}"
}
})" };

VerifyParseSucceeded(settings0String);
VerifyParseSucceeded(settings1String);
VerifyParseSucceeded(settings2String);
VerifyParseSucceeded(settings3String);
VerifyParseSucceeded(settings4String);
VerifyParseSucceeded(settings5String);
const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");

{
CascadiaSettings settings;
settings._ParseJsonString(settings0String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid1, settings._globals._defaultProfile);
VERIFY_ARE_EQUAL(123, settings._globals._initialRows);
}
{
CascadiaSettings settings;
settings._ParseJsonString(settings1String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid1, settings._globals._defaultProfile);
VERIFY_ARE_EQUAL(234, settings._globals._initialRows);
}
{
CascadiaSettings settings;
settings._ParseJsonString(settings2String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid1, settings._globals._defaultProfile);
VERIFY_ARE_EQUAL(345, settings._globals._initialRows);
}
{
CascadiaSettings settings;
settings._ParseJsonString(settings3String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid2, settings._globals._defaultProfile);
VERIFY_ARE_EQUAL(456, settings._globals._initialRows);
}
{
CascadiaSettings settings;
settings._ParseJsonString(settings4String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid1, settings._globals._defaultProfile);
}
{
CascadiaSettings settings;
settings._ParseJsonString(settings5String, false);
settings.LayerJson(settings._userSettings);
VERIFY_ARE_EQUAL(guid3, settings._globals._defaultProfile);
}
}

}
12 changes: 5 additions & 7 deletions src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,20 +457,18 @@ std::unique_ptr<CascadiaSettings> CascadiaSettings::FromJson(const Json::Value&
// <none>
void CascadiaSettings::LayerJson(const Json::Value& json)
{
// microsoft/terminal#2906: First layer the root object as our globals. If
// there is also a `globals` object, layer that one on top of the settings
// from the root.
_globals.LayerJson(json);

if (auto globals{ json[GlobalsKey.data()] })
{
if (globals.isObject())
{
_globals.LayerJson(globals);
}
}
else
{
// If there's no globals key in the root object, then try looking at the
// root object for those properties instead, to gracefully upgrade.
// This will attempt to do the legacy keybindings loading too
_globals.LayerJson(json);
}

if (auto schemes{ json[SchemesKey.data()] })
{
Expand Down