Always create and link profiles.defaults object (#8445)

The Settings UI exposes the `profiles.defaults` (PD) object. Today, we remove PD if there's nothing in it. However, that causes problems with the Settings UI, because we have no `Profile` object to bind to (resulting in a crash). Rather than making the Settings UI create a PD, and link it in the inheritance tree, it's much easier to just _always_ create and link the PD object. ## References #1564 - Settings UI (fixes a crash for this) #7923 - Introduces inheritance ## PR Checklist * [X] Tests added/passed ## Validation Steps Performed * [x] repro steps for crash in Settings UI (copied changes over to that branch for testing) * [x] tests passed
microsoft · Dec 1, 2020 · 6213172 · 6213172
1 parent 4060a18
commit 6213172
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 26 deletions.
diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
@@ -2568,17 +2568,17 @@ namespace SettingsModelLocalTests
             const auto copy{ settings->Copy() };
             const auto copyImpl{ winrt::get_self<implementation::CascadiaSettings>(copy) };
 
-            // test optimization: if we don't have profiles.defaults, don't add it to the tree
-            VERIFY_IS_NULL(settings->_userDefaultProfileSettings);
-            VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings, copyImpl->_userDefaultProfileSettings);
+            // if we don't have profiles.defaults, it should still be in the tree
+            VERIFY_IS_NOT_NULL(settings->_userDefaultProfileSettings);
+            VERIFY_IS_NOT_NULL(copyImpl->_userDefaultProfileSettings);
 
             VERIFY_ARE_EQUAL(settings->ActiveProfiles().Size(), 1u);
             VERIFY_ARE_EQUAL(settings->ActiveProfiles().Size(), copyImpl->ActiveProfiles().Size());
 
             // so we should only have one parent, instead of two
             auto srcProfile{ winrt::get_self<implementation::Profile>(settings->ActiveProfiles().GetAt(0)) };
             auto copyProfile{ winrt::get_self<implementation::Profile>(copyImpl->ActiveProfiles().GetAt(0)) };
-            VERIFY_ARE_EQUAL(srcProfile->Parents().size(), 0u);
+            VERIFY_ARE_EQUAL(srcProfile->Parents().size(), 1u);
             VERIFY_ARE_EQUAL(srcProfile->Parents().size(), copyProfile->Parents().size());
         };
 

diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -732,7 +732,8 @@ void CascadiaSettings::_ApplyDefaultsFromUserSettings()
 {
     // If `profiles` was an object, then look for the `defaults` object
     // underneath it for the default profile settings.
-    auto defaultSettings{ Json::Value::null };
+    // If there isn't one, we still want to add an empty "default" profile to the inheritance tree.
+    Json::Value defaultSettings{ Json::ValueType::objectValue };
     if (const auto profiles{ _userSettings[JsonKey(ProfilesKey)] })
     {
         if (profiles.isObject() && !profiles[JsonKey(DefaultSettingsKey)].empty())
@@ -741,31 +742,26 @@ void CascadiaSettings::_ApplyDefaultsFromUserSettings()
         }
     }
 
-    // cache and apply default profile settings
-    // from user settings file
-    if (defaultSettings)
-    {
-        // Remove the `guid` member from the default settings. That'll
-        // hyper-explode, so just don't let them do that.
-        defaultSettings.removeMember({ "guid" });
+    // Remove the `guid` member from the default settings. That'll
+    // hyper-explode, so just don't let them do that.
+    defaultSettings.removeMember({ "guid" });
 
-        _userDefaultProfileSettings = winrt::make_self<Profile>();
-        _userDefaultProfileSettings->LayerJson(defaultSettings);
+    _userDefaultProfileSettings = winrt::make_self<Profile>();
+    _userDefaultProfileSettings->LayerJson(defaultSettings);
 
-        const auto numOfProfiles{ _allProfiles.Size() };
-        for (uint32_t profileIndex = 0; profileIndex < numOfProfiles; ++profileIndex)
-        {
-            // create a child, so we inherit from the defaults.json layer
-            auto parentProj{ _allProfiles.GetAt(profileIndex) };
-            auto parentImpl{ winrt::get_self<Profile>(parentProj) };
-            auto childImpl{ parentImpl->CreateChild() };
+    const auto numOfProfiles{ _allProfiles.Size() };
+    for (uint32_t profileIndex = 0; profileIndex < numOfProfiles; ++profileIndex)
+    {
+        // create a child, so we inherit from the defaults.json layer
+        auto parentProj{ _allProfiles.GetAt(profileIndex) };
+        auto parentImpl{ winrt::get_self<Profile>(parentProj) };
+        auto childImpl{ parentImpl->CreateChild() };
 
-            // Add profile.defaults as the _first_ parent to the child
-            childImpl->InsertParent(0, _userDefaultProfileSettings);
+        // Add profile.defaults as the _first_ parent to the child
+        childImpl->InsertParent(0, _userDefaultProfileSettings);
 
-            // replace parent in _profiles with child
-            _allProfiles.SetAt(profileIndex, *childImpl);
-        }
+        // replace parent in _profiles with child
+        _allProfiles.SetAt(profileIndex, *childImpl);
     }
 }