simplify IInheritable macros and comments

carlos-zamora · carlos-zamora · commit 956a05926314 · 2026-06-04T14:26:19.000-07:00
diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp
@@ -242,13 +242,8 @@ void AppearanceConfig::LayerJson(const Json::Value& json)
     _logSettingIfSet(OpacityKey, HasOpacity());
 
     // ColorScheme: translate the polymorphic on-disk "colorScheme" key (string, or
-    // { "dark", "light" }) into the two independent internal keys. A string sets both sides;
-    // an object sets only the side(s) present (the missing side is left untouched so it
-    // inherits / retains a prior layer's value). Matches main's LayerJson behavior.
-    // ColorScheme: translate the polymorphic on-disk "colorScheme" key (string, or
-    // { "dark", "light" }) into the two independent internal keys. A string sets both sides;
-    // an object sets only the side(s) present (the missing side is left untouched so it
-    // inherits / retains a prior layer's value). Matches main's LayerJson behavior.
+    // { "dark", "light" }) into the two independent internal keys. A string sets both
+    // sides; an object sets only the side(s) present, leaving the other to inherit.
     if (const auto& colorScheme{ json[JsonKey(ColorSchemeKey)] }; colorScheme.isString())
     {
         _json[JsonKey(DarkColorSchemeNameKey)] = colorScheme;
diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h
diff --git a/src/cascadia/TerminalSettingsModel/JsonSyncCollections.h b/src/cascadia/TerminalSettingsModel/JsonSyncCollections.h
@@ -6,33 +6,15 @@ Module Name:
 - JsonSyncCollections.h
 
 Abstract:
-- IVector / IMap wrapper templates that mirror in-place mutations into a
-  parent settings object's Json::Value _json. Required for "hybrid" settings
-  whose JSON storage would otherwise miss .Append / .InsertAt / .Insert /
-  .RemoveAt / etc. issued by the settings editor on the returned container.
-
-  Two backing flavors are supported, both via the same wrapper template:
-
-  1. Ephemeral shadow: caller passes a freshly-constructed IVector / IMap
-     (seeded from the effective JSON value) as the backing. The wrapper is
-     short-lived and discarded after use. _json is the only persistent
-     source of truth. Used for DisabledProfileSources, FontAxes,
-     FontFeatures, EnvironmentVariables.
-
-  2. Persistent backing: caller passes an existing long-lived IVector / IMap
-     field on the parent (e.g. Profile::_BellSound) as the backing. The
-     wrapper dual-writes to that field AND to _json. Used for BellSound,
-     where IMediaResource resolution state must persist across getter calls.
-
-  The wrapper invokes an onChange callback after every mutation. The
-  callback typically captures a strong ref to the parent and re-serializes
-  the current backing into _json[key] via JsonUtils::SetValueForKey, then
-  fires the WriteSettings signal (TODO GH#12424).
-
-  These templates intentionally implement IVector / IMap only — not
-  IObservableVector / IObservableMap. No editor call site subscribes to
-  VectorChanged / MapChanged on the returned collection; subscribers live
-  on parallel VM-side observable collections.
+- IVector / IMap wrapper templates that mirror in-place mutations (.Append,
+  .InsertAt, .RemoveAt, .Insert, …) into a parent settings object's _json via an
+  onChange callback. Without this, editor mutations on a returned container would
+  be lost. The backing may be an ephemeral shadow seeded from _json (the common
+  case; _json is the source of truth) or a persistent field whose state must
+  outlive the call (e.g. Profile::_BellSound, which holds IMediaResource
+  resolution state).
+- IVector / IMap only — not the IObservable variants; nothing subscribes to
+  VectorChanged / MapChanged here (VM-side collections handle that).
 
 Author(s):
 - Carlos Zamora - June 2026
diff --git a/src/cascadia/TerminalSettingsModel/NewTabMenu.cpp b/src/cascadia/TerminalSettingsModel/NewTabMenu.cpp
@@ -121,7 +121,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         _entries = std::nullopt;
         _json = Json::Value{ Json::arrayValue };
-        // TODO GH#12424: raise WriteSettings event
+        // TODO GH#17000: raise WriteSettings event
     }
 
     void NewTabMenu::ReplaceAll(const IVector<Model::NewTabMenuEntry>& entries)
@@ -377,7 +377,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         }
         _json = std::move(arr);
         _emitChangeLog();
-        // TODO GH#12424: raise WriteSettings event
+        // TODO GH#17000: raise WriteSettings event
     }
 
     void NewTabMenu::_emitChangeLog()
diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h
@@ -134,7 +134,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         // Nullable/optional settings (JSON-backed)
         INHERITABLE_NULLABLE_SETTING(Model::Profile, Microsoft::Terminal::Core::Color, TabColor, "tabColor", nullptr)
-        INHERITABLE_MUTABLE_SETTING(Model::Profile, Model::IAppearanceConfig, UnfocusedAppearance, nullptr);
+
+        // UnfocusedAppearance holds a live AppearanceConfig sub-object whose
+        // presence is the marker; it needs a backing field (not _json).
+        _BASE_INHERITABLE_MUTABLE_SETTING(Model::Profile, std::optional<Model::IAppearanceConfig>, UnfocusedAppearance, nullptr)
+    public:
+        Model::IAppearanceConfig UnfocusedAppearance() const
+        {
+            const auto val{ _getUnfocusedAppearanceImpl() };
+            return val ? *val : Model::IAppearanceConfig{ nullptr };
+        }
+        void UnfocusedAppearance(const Model::IAppearanceConfig& value)
+        {
+            _UnfocusedAppearance = value;
+        }
+        void ClearUnfocusedAppearance()
+        {
+            _UnfocusedAppearance = std::nullopt;
+        }
 
         // Settings that are JSON-backed but need custom handling in ToJson/LayerJson
         INHERITABLE_SETTING(Model::Profile, hstring, Name, "name", L"Default")
diff --git a/src/cascadia/UnitTests_SettingsModel/JsonSyncCollectionsTests.cpp b/src/cascadia/UnitTests_SettingsModel/JsonSyncCollectionsTests.cpp
@@ -0,0 +1,176 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#include "pch.h"
+
+#include "../TerminalSettingsModel/JsonSyncCollections.h"
+
+using namespace winrt::Windows::Foundation::Collections;
+using namespace winrt::Microsoft::Terminal::Settings::Model::implementation;
+using namespace WEX::Logging;
+using namespace WEX::TestExecution;
+
+namespace SettingsModelUnitTests
+{
+    // Unit tests for the JsonSyncVector / JsonSyncMap wrappers in isolation
+    // (with a stub onChange callback), independent of any settings object.
+    class JsonSyncCollectionsTests
+    {
+        TEST_CLASS(JsonSyncCollectionsTests);
+
+        TEST_METHOD(VectorReadsDelegateToBacking);
+        TEST_METHOD(VectorMutationsEachFireOnChangeOnce);
+        TEST_METHOD(VectorOnChangeReceivesMutatedBacking);
+        TEST_METHOD(VectorIterationWorks);
+        TEST_METHOD(VectorNullOnChangeIsSafe);
+
+        TEST_METHOD(MapReadsDelegateToBacking);
+        TEST_METHOD(MapMutationsEachFireOnChangeOnce);
+        TEST_METHOD(MapOnChangeReceivesMutatedBacking);
+    };
+
+    void JsonSyncCollectionsTests::VectorReadsDelegateToBacking()
+    {
+        const auto backing = winrt::single_threaded_vector<winrt::hstring>({ L"a", L"b", L"c" });
+        auto fired = 0;
+        const IVector<winrt::hstring> wrapper = winrt::make<JsonSyncVector<winrt::hstring>>(
+            backing, [&](IVector<winrt::hstring> const&) { ++fired; });
+
+        VERIFY_ARE_EQUAL(3u, wrapper.Size());
+        VERIFY_ARE_EQUAL(L"b", wrapper.GetAt(1));
+
+        uint32_t index{};
+        VERIFY_IS_TRUE(wrapper.IndexOf(L"c", index));
+        VERIFY_ARE_EQUAL(2u, index);
+
+        VERIFY_ARE_EQUAL(3u, wrapper.GetView().Size());
+
+        // Reads must not trigger the sync callback.
+        VERIFY_ARE_EQUAL(0, fired);
+    }
+
+    void JsonSyncCollectionsTests::VectorMutationsEachFireOnChangeOnce()
+    {
+        const auto backing = winrt::single_threaded_vector<winrt::hstring>({ L"a" });
+        auto fired = 0;
+        const IVector<winrt::hstring> wrapper = winrt::make<JsonSyncVector<winrt::hstring>>(
+            backing, [&](IVector<winrt::hstring> const&) { ++fired; });
+
+        wrapper.Append(L"b");
+        VERIFY_ARE_EQUAL(1, fired);
+
+        wrapper.InsertAt(0, L"z");
+        VERIFY_ARE_EQUAL(2, fired);
+
+        wrapper.SetAt(0, L"y");
+        VERIFY_ARE_EQUAL(3, fired);
+
+        wrapper.RemoveAt(0);
+        VERIFY_ARE_EQUAL(4, fired);
+
+        wrapper.RemoveAtEnd();
+        VERIFY_ARE_EQUAL(5, fired);
+
+        const std::array<winrt::hstring, 2> replacement{ L"p", L"q" };
+        wrapper.ReplaceAll(replacement);
+        VERIFY_ARE_EQUAL(6, fired);
+        VERIFY_ARE_EQUAL(2u, wrapper.Size());
+
+        wrapper.Clear();
+        VERIFY_ARE_EQUAL(7, fired);
+        VERIFY_ARE_EQUAL(0u, wrapper.Size());
+    }
+
+    void JsonSyncCollectionsTests::VectorOnChangeReceivesMutatedBacking()
+    {
+        const auto backing = winrt::single_threaded_vector<winrt::hstring>({ L"a" });
+        uint32_t lastSize{};
+        const IVector<winrt::hstring> wrapper = winrt::make<JsonSyncVector<winrt::hstring>>(
+            backing, [&](IVector<winrt::hstring> const& current) { lastSize = current.Size(); });
+
+        wrapper.Append(L"b");
+        // The callback sees the already-mutated backing, and the mutation lands
+        // on the shared backing the caller passed in.
+        VERIFY_ARE_EQUAL(2u, lastSize);
+        VERIFY_ARE_EQUAL(2u, backing.Size());
+        VERIFY_ARE_EQUAL(L"b", backing.GetAt(1));
+    }
+
+    void JsonSyncCollectionsTests::VectorIterationWorks()
+    {
+        const auto backing = winrt::single_threaded_vector<winrt::hstring>({ L"x", L"y" });
+        const IVector<winrt::hstring> wrapper = winrt::make<JsonSyncVector<winrt::hstring>>(
+            backing, nullptr);
+
+        std::vector<winrt::hstring> seen;
+        for (const auto& item : wrapper)
+        {
+            seen.push_back(item);
+        }
+        VERIFY_ARE_EQUAL(2u, static_cast<uint32_t>(seen.size()));
+        VERIFY_ARE_EQUAL(L"x", seen[0]);
+        VERIFY_ARE_EQUAL(L"y", seen[1]);
+    }
+
+    void JsonSyncCollectionsTests::VectorNullOnChangeIsSafe()
+    {
+        const auto backing = winrt::single_threaded_vector<winrt::hstring>();
+        const IVector<winrt::hstring> wrapper = winrt::make<JsonSyncVector<winrt::hstring>>(
+            backing, nullptr);
+
+        // A null onChange must be tolerated (the wrapper guards the callback).
+        wrapper.Append(L"a");
+        VERIFY_ARE_EQUAL(1u, wrapper.Size());
+    }
+
+    void JsonSyncCollectionsTests::MapReadsDelegateToBacking()
+    {
+        const auto backing = winrt::single_threaded_map<winrt::hstring, winrt::hstring>();
+        backing.Insert(L"k1", L"v1");
+
+        auto fired = 0;
+        const IMap<winrt::hstring, winrt::hstring> wrapper = winrt::make<JsonSyncMap<winrt::hstring, winrt::hstring>>(
+            backing, [&](IMap<winrt::hstring, winrt::hstring> const&) { ++fired; });
+
+        VERIFY_ARE_EQUAL(1u, wrapper.Size());
+        VERIFY_IS_TRUE(wrapper.HasKey(L"k1"));
+        VERIFY_IS_FALSE(wrapper.HasKey(L"nope"));
+        VERIFY_ARE_EQUAL(L"v1", wrapper.Lookup(L"k1"));
+        VERIFY_ARE_EQUAL(1u, wrapper.GetView().Size());
+
+        VERIFY_ARE_EQUAL(0, fired);
+    }
+
+    void JsonSyncCollectionsTests::MapMutationsEachFireOnChangeOnce()
+    {
+        const auto backing = winrt::single_threaded_map<winrt::hstring, winrt::hstring>();
+        auto fired = 0;
+        const IMap<winrt::hstring, winrt::hstring> wrapper = winrt::make<JsonSyncMap<winrt::hstring, winrt::hstring>>(
+            backing, [&](IMap<winrt::hstring, winrt::hstring> const&) { ++fired; });
+
+        VERIFY_IS_FALSE(wrapper.Insert(L"k1", L"v1")); // new key → not a replacement
+        VERIFY_ARE_EQUAL(1, fired);
+
+        VERIFY_IS_TRUE(wrapper.Insert(L"k1", L"v2")); // existing key → replacement
+        VERIFY_ARE_EQUAL(2, fired);
+
+        wrapper.Remove(L"k1");
+        VERIFY_ARE_EQUAL(3, fired);
+
+        wrapper.Clear();
+        VERIFY_ARE_EQUAL(4, fired);
+    }
+
+    void JsonSyncCollectionsTests::MapOnChangeReceivesMutatedBacking()
+    {
+        const auto backing = winrt::single_threaded_map<winrt::hstring, winrt::hstring>();
+        uint32_t lastSize{};
+        const IMap<winrt::hstring, winrt::hstring> wrapper = winrt::make<JsonSyncMap<winrt::hstring, winrt::hstring>>(
+            backing, [&](IMap<winrt::hstring, winrt::hstring> const& current) { lastSize = current.Size(); });
+
+        wrapper.Insert(L"k1", L"v1");
+        VERIFY_ARE_EQUAL(1u, lastSize);
+        VERIFY_ARE_EQUAL(1u, backing.Size());
+        VERIFY_ARE_EQUAL(L"v1", backing.Lookup(L"k1"));
+    }
+}
diff --git a/src/cascadia/UnitTests_SettingsModel/ProfileTests.cpp b/src/cascadia/UnitTests_SettingsModel/ProfileTests.cpp
@@ -51,7 +51,7 @@ namespace SettingsModelUnitTests
 
         TEST_METHOD(ClearIconAlsoClearsJson);
         TEST_METHOD(ClearIconAndBellSoundAreLogged);
-        
+
         TEST_METHOD(SettingInheritanceFallback);
         TEST_METHOD(ClearSettingRestoresInheritance);
         TEST_METHOD(HasSettingAtSpecificLayer);
@@ -844,7 +844,7 @@ namespace SettingsModelUnitTests
         // NOTE: CascadiaSettings::_validateAllSchemesExist() runs during construction and
         // clears any DarkColorSchemeName/LightColorSchemeName that doesn't name a registered
         // scheme (falling back to the default). So every scheme the cases below reference must
-        // be registered via this inbox JSON, otherwise the loaded values get wiped on load.
+        // be registered via this inbox JSON; otherwise the loaded values get wiped on load.
         static constexpr std::string_view inboxSchemes{ R"({
             "schemes": [
                 { "name": "Campbell",       "black": "#0C0C0C", "red": "#C50F1F", "green": "#13A10E", "yellow": "#C19C00", "blue": "#0037DA", "purple": "#881798", "cyan": "#3A96DD", "white": "#CCCCCC", "brightBlack": "#767676", "brightRed": "#E74856", "brightGreen": "#16C60C", "brightYellow": "#F9F1A5", "brightBlue": "#3B78FF", "brightPurple": "#B4009E", "brightCyan": "#61D6D6", "brightWhite": "#F2F2F2" },
@@ -1390,7 +1390,7 @@ namespace SettingsModelUnitTests
         VERIFY_IS_TRUE(changes.contains("profile.bellSound"));
     }
 
-        void ProfileTests::SettingInheritanceFallback()
+    void ProfileTests::SettingInheritanceFallback()
     {
         // Verify that when no layer defines a setting, the default value is used.
         // Also verify that when only user defaults defines it, profiles inherit from there.
diff --git a/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj b/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj
@@ -40,6 +40,7 @@
     <ClCompile Include="KeyBindingsTests.cpp" />
     <ClCompile Include="CommandTests.cpp" />
     <ClCompile Include="DeserializationTests.cpp" />
+    <ClCompile Include="JsonSyncCollectionsTests.cpp" />
     <ClCompile Include="NewTabMenuTests.cpp" />
     <ClCompile Include="SerializationTests.cpp" />
     <ClCompile Include="TerminalSettingsTests.cpp" />

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation`
`121`	`121`	`{`
`122`	`122`	`_entries = std::nullopt;`
`123`	`123`	`_json = Json::Value{ Json::arrayValue };`
`124`		`- // TODO GH#12424: raise WriteSettings event`
	`124`	`+ // TODO GH#17000: raise WriteSettings event`
`125`	`125`	`}`
`126`	`126`
`127`	`127`	`void NewTabMenu::ReplaceAll(const IVector<Model::NewTabMenuEntry>& entries)`
`@@ -377,7 +377,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation`
`377`	`377`	`}`
`378`	`378`	`_json = std::move(arr);`
`379`	`379`	`_emitChangeLog();`
`380`		`- // TODO GH#12424: raise WriteSettings event`
	`380`	`+ // TODO GH#17000: raise WriteSettings event`
`381`	`381`	`}`
`382`	`382`
`383`	`383`	`void NewTabMenu::_emitChangeLog()`