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

## Summary of the Pull Request Layer the `globals` globals on top of the root globals. ## PR Checklist * [x] Closes #2906 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## 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.
microsoft · Oct 4, 2019 · 0691c21 · 0691c21
1 parent 94fc40e
commit 0691c21
Show file tree

Hide file tree

Showing 2 changed files with 117 additions and 7 deletions.
diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
@@ -49,6 +49,8 @@ namespace TerminalAppLocalTests
         TEST_METHOD(TestExplodingNameOnlyProfiles);
         TEST_METHOD(TestHideAllProfiles);
 
+        TEST_METHOD(TestLayerGlobalsOnRoot);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             InitializeJsonReader();
@@ -1191,4 +1193,114 @@ 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"(
+        {
+            "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);
+        }
+    }
+
 }
diff --git a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
@@ -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()] })
     {