Layer the globals globals on top of the root globals #3031
Conversation
zadjii-msft
added
Area-Settings
| // initialRows should not be cleared here | ||
| } | ||
| })" }; | ||
| const std::string settings3String{ R"( |
There was a problem hiding this comment.
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}.
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
Oh, i see: file level ordering. Interesting.
...
we can check getOffsetStart and getOffsetLimit 😉
There was a problem hiding this comment.
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
ghost
added
Needs-Author-Feedback
| @@ -1191,4 +1193,81 @@ namespace TerminalAppLocalTests | |||
| VERIFY_IS_TRUE(caughtExpectedException); | |||
| } | |||
| } | |||
|
|
|||
| void SettingsTests::TestLayerGlobalsOnRoot() | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
this is just gonna be invalid
There was a problem hiding this comment.
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.
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 :(
|
This is giving me a headache, so I'm going to let Carlos and Dustin be the reviewers as they seem to have a handle on it already. @ me if you really need me to look at this. |
Summary of the Pull Request
Layer the
globalsglobals on top of the root globals.PR Checklist
Detailed Description of the Pull Request / Additional comments
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 will get ignored. That's bad. We should layer them.