From 84e7ec4f96d1b83a177c5549b8f1e944f0fed20e Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Thu, 7 Oct 2021 18:30:34 +0200
Subject: [PATCH] Fix layering issues with CascadiaSettings::_createNewProfile
 (#11447)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`CascadiaSettings::_createNewProfile` failed to call `_FinalizeInheritance`.
This commits fixes the issue and adds a stern warning for future me.

## PR Checklist
* [x] Closes #11392
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Open settings UI
* Modify font size in base layer
* _Don't_ save
* Duplicate any profile with default font size
* Ensure the duplicated profile shows the modified base layer font size ✔️
---
 .../LocalTests_SettingsModel/ProfileTests.cpp | 26 +++++++++++++------
 .../CascadiaSettings.cpp                      |  8 ++++++
 .../CascadiaSettingsSerialization.cpp         |  1 +
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
index ee42b89a4..2c5138f6d 100644
--- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
@@ -272,20 +272,30 @@ namespace SettingsModelLocalTests
     void ProfileTests::DuplicateProfileTest()
     {
         static constexpr std::string_view userProfiles{ R"({
-            "profiles": [
-                {
-                    "name": "profile0",
-                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
-                    "backgroundImage": "file:///some/path",
-                    "hidden": false,
-                }
-            ]
+            "profiles": {
+                "defaults": {
+                    "font": {
+                        "size": 123
+                    }
+                },
+                "list": [
+                    {
+                        "name": "profile0",
+                        "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                        "backgroundImage": "file:///some/path",
+                        "hidden": false,
+                    }
+                ]
+            }
         })" };
 
         const auto settings = winrt::make_self<implementation::CascadiaSettings>(userProfiles);
         const auto profile = settings->AllProfiles().GetAt(0);
         const auto duplicatedProfile = settings->DuplicateProfile(profile);
 
+        // GH#11392: Ensure duplicated profiles properly inherit the base layer, even for nested objects.
+        VERIFY_ARE_EQUAL(123, duplicatedProfile.FontInfo().FontSize());
+
         duplicatedProfile.Guid(profile.Guid());
         duplicatedProfile.Name(profile.Name());
 
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index 4a9b274bf..ac7008416 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
@@ -18,6 +18,13 @@ using namespace winrt::Microsoft::Terminal::Control;
 using namespace winrt::Windows::Foundation::Collections;
 using namespace Microsoft::Console;
 
+// Creating a child of a profile requires us to copy certain
+// required attributes. This method handles those attributes.
+//
+// NOTE however that it doesn't call _FinalizeInheritance() for you! Don't forget that!
+//
+// At the time of writing only one caller needs to call _FinalizeInheritance(),
+// which is why this unsafety wasn't further abstracted away.
 winrt::com_ptr<Profile> Model::implementation::CreateChild(const winrt::com_ptr<Profile>& parent)
 {
     auto profile = winrt::make_self<Profile>();
@@ -371,6 +378,7 @@ winrt::com_ptr<Profile> CascadiaSettings::_createNewProfile(const std::wstring_v
     LOG_IF_FAILED(CoCreateGuid(&guid));
 
     auto profile = CreateChild(_baseLayerProfile);
+    profile->_FinalizeInheritance();
     profile->Guid(guid);
     profile->Name(winrt::hstring{ name });
     return profile;
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index 224fe2851..c0e925c56 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -596,6 +596,7 @@ void SettingsLoader::_addParentProfile(const winrt::com_ptr<implementation::Prof
         //
         // When a user modifies a profile they shouldn't modify the (static/constant)
         // inbox profile of course. That's why we need to call CreateChild here.
+        // But we don't need to call _FinalizeInheritance() yet as this is handled by SettingsLoader::FinalizeLayering().
         settings.profiles.emplace_back(CreateChild(profile));
     }
 }