Repository: microsoft/terminal (language: cpp)

Issue:
Partners cannot rename a template via fragment plugins
### Windows Terminal version

_No response_

### Windows build number

_No response_

### Other Software

_No response_

### Steps to reproduce

Ship a template fragment with something like:
```
"profiles": [
      {
        "updates": "{f184b75f-6c82-5ea5-9b3e-3203d66c5f82}",
        "name": "My new profile name",
      },
    ]
  }
```
Then restart the terminal to take the new fragment in account.

### Expected Behavior

"My new profile name" should appear as the profile name

### Actual Behavior

The old name still appears, "name" in case of "update" is discared.

The rationale here is to replace the generated named in WSL, which is using the registered WSL ID (`Ubuntu2004LTS` for instance) instead of the Display Name registered (and used as the tab name entry) like the more readable `Ubuntu 20.04 LTS`.
Other kind of entries are overridden by the fragment, but not the `name` one.

The workaround right now is to hide the generated template with `hidden: true` and add a whole new entry.


Discussion / hints:
Huh. It should be able to do that. Why doesn't that work? I bet this regressed in one of the many settings refactors over the last few months. 

Thanks for filing this! Let's make sure to get this fixed. 

/cc @lhecker for this one ;)
I'll debug this issue starting next week. 🙂
The problem here is that fields in a user's settings.json take precedence over fragments. Since profiles are created and written with a "name" field, the "name" field in fragments usually has no effect. If you were to remove the "name" field from your settings.json this would work as expected.

What is unexpected however is that we don't respect such updates when the dynamic profile is first added. I'm going to submit a PR for that.

Fix patch:
diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
index c20a9a853e4..a623a204204 100644
--- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
@@ -1983,7 +1983,7 @@ namespace SettingsModelLocalTests
         }
     }
 
-    // This test ensures GH#11597 doesn't regress.
+    // This test ensures GH#11597, GH#12520 don't regress.
     void DeserializationTests::LoadFragmentsWithMultipleUpdates()
     {
         static constexpr std::wstring_view fragmentSource{ L"fragment" };
@@ -1991,7 +1991,7 @@ namespace SettingsModelLocalTests
             "profiles": [
                 {
                     "updates": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
-                    "cursorShape": "filledBox"
+                    "name": "NewName"
                 },
                 {
                     "updates": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
@@ -2011,5 +2011,7 @@ namespace SettingsModelLocalTests
 
         VERIFY_IS_FALSE(loader.duplicateProfile);
         VERIFY_ARE_EQUAL(3u, loader.userSettings.profiles.size());
+        // GH#12520: Fragments should be able to override the name of builtin profiles.
+        VERIFY_ARE_EQUAL(L"NewName", loader.userSettings.profiles[0]->Name());
     }
 }
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index b3f49688a26..55456867f91 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
@@ -31,6 +31,8 @@ using namespace Microsoft::Console;
 // which is why this unsafety wasn't further abstracted away.
 winrt::com_ptr<Profile> Model::implementation::CreateChild(const winrt::com_ptr<Profile>& parent)
 {
+    // If you add more fields here, make sure to do the same in
+    // SettingsLoader::_addUserProfileParent().
     auto profile = winrt::make_self<Profile>();
     profile->Origin(OriginTag::User);
     profile->Name(parent->Name());
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
index 772f7625beb..381a5148ce6 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
@@ -84,7 +84,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         static JsonSettings _parseJson(const std::string_view& content);
         static winrt::com_ptr<implementation::Profile> _parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson);
         void _appendProfile(winrt::com_ptr<Profile>&& profile, const winrt::guid& guid, ParsedSettings& settings);
-        static void _addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings);
+        void _addUserProfileParent(const winrt::com_ptr<implementation::Profile>& profile);
         void _executeGenerator(const IDynamicProfileGenerator& generator);
 
         std::unordered_set<std::wstring_view> _ignoredNamespaces;
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index 87a7ffdca8d..001c52d34b1 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -183,7 +183,7 @@ void SettingsLoader::MergeInboxIntoUserSettings()
 {
     for (const auto& profile : inboxSettings.profiles)
     {
-        _addParentProfile(profile, userSettings);
+        _addUserProfileParent(profile);
     }
 }
 
@@ -293,7 +293,17 @@ void SettingsLoader::FinalizeLayering()
     for (const auto& profile : userSettings.profiles)
     {
         profile->AddMostImportantParent(userSettings.baseLayerProfile);
+
+        // This completes the parenting process that was started in _addUserProfileParent().
         profile->_FinalizeInheritance();
+        if (profile->Origin() == OriginTag::None)
+        {
+            // If you add more fields here, make sure to do the same in
+            // implementation::CreateChild().
+            profile->Origin(OriginTag::User);
+            profile->Name(profile->Name());
+            profile->Hidden(profile->Hidden());
+        }
     }
 }
 
@@ -529,7 +539,7 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
         }
         else
         {
-            _addParentProfile(fragmentProfile, userSettings);
+            _addUserProfileParent(fragmentProfile);
         }
     }
 
@@ -592,9 +602,9 @@ void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, const win
 
 // If the given ParsedSettings instance contains a profile with the given profile's GUID,
 // the profile is added as a parent. Otherwise a new child profile is created.
-void SettingsLoader::_addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings)
+void SettingsLoader::_addUserProfileParent(const winrt::com_ptr<implementation::Profile>& profile)
 {
-    if (const auto [it, inserted] = settings.profilesByGuid.emplace(profile->Guid(), profile); !inserted)
+    if (const auto [it, inserted] = userSettings.profilesByGuid.emplace(profile->Guid(), nullptr); !inserted)
     {
         // If inserted is false, we got a matching user profile with identical GUID.
         // --> The generated profile is a parent of the existing user profile.
@@ -602,14 +612,36 @@ void SettingsLoader::_addParentProfile(const winrt::com_ptr<implementation::Prof
     }
     else
     {
-        // If inserted is true, then this is a generated profile that doesn't exist in the user's settings.
-        // While emplace() has already created an appropriate entry in .profilesByGuid, we still need to
-        // add it to .profiles (which is basically a sorted list of .profilesByGuid's values).
+        // If inserted is true, then this is a generated profile that doesn't exist
+        // in the user's settings (which makes this branch somewhat unlikely).
         //
         // 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));
+        // inbox profile of course. That's why we need to create a child.
+        // And since we previously added the (now) parent profile into profilesByGuid
+        // we'll have to replace it->second with the (new) child profile.
+        //
+        // These additional things are required to complete a (user) profile:
+        // * A call to _FinalizeInheritance()
+        // * Every profile should at least have Origin(), Name() and Hidden() set
+        // They're handled by SettingsLoader::FinalizeLayering() and detected by
+        // the missing Origin(). Setting these fields as late as possible ensures
+        // that we pick up the correct, inherited values of all of the child's parents.
+        //
+        // If you add more fields here, make sure to do the same in
+        // implementation::CreateChild().
+        auto child = winrt::make_self<Profile>();
+        child->AddLeastImportantParent(profile);
+        child->Guid(profile->Guid());
+
+        // If profile is a dynamic/generated profile, a fragment's
+        // Source() should have no effect on this user profile.
+        if (profile->HasSource())
+        {
+            child->Source(profile->Source());
+        }
+
+        it->second = child;
+        userSettings.profiles.emplace_back(std::move(child));
     }
 }
 


Test patch: