-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Layer the globals globals on top of the root globals #3031
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,8 @@ namespace TerminalAppLocalTests | |
TEST_METHOD(TestExplodingNameOnlyProfiles); | ||
TEST_METHOD(TestHideAllProfiles); | ||
|
||
TEST_METHOD(TestLayerGlobalsOnRoot); | ||
|
||
TEST_CLASS_SETUP(ClassSetup) | ||
{ | ||
InitializeJsonReader(); | ||
|
@@ -1191,4 +1193,81 @@ namespace TerminalAppLocalTests | |
VERIFY_IS_TRUE(caughtExpectedException); | ||
} | ||
} | ||
|
||
void SettingsTests::TestLayerGlobalsOnRoot() | ||
{ | ||
// 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"( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add another test:
I think as of now, processing this will make defaultProfile There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Think it might be worth creating an issue then and just leaving it for later? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 | ||
} | ||
})" }; | ||
|
||
VerifyParseSucceeded(settings0String); | ||
VerifyParseSucceeded(settings1String); | ||
VerifyParseSucceeded(settings2String); | ||
VerifyParseSucceeded(settings3String); | ||
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}"); | ||
|
||
{ | ||
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); | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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:This might be impacted by the other thread I started...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😁There was a problem hiding this comment.
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 :(