From 7d6b7d3f1fb4b3c626a3cb9370daa0f007cbc636 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 5 Jun 2026 16:21:23 -0700 Subject: [PATCH] Centralize settings write notification logic --- .../ActionsViewModel.cpp | 20 +++++ .../TerminalSettingsEditor/ActionsViewModel.h | 1 + .../TerminalSettingsModel/ActionMap.cpp | 16 ++-- .../TerminalSettingsModel/ActionMap.idl | 1 + .../CascadiaSettingsSerialization.cpp | 8 ++ .../TerminalSettingsModel/ColorScheme.cpp | 2 + .../TerminalSettingsModel/ColorScheme.h | 3 +- .../GlobalAppSettings.cpp | 13 ++- .../TerminalSettingsModel/IInheritable.h | 81 +++++-------------- ...crosoft.Terminal.Settings.ModelLib.vcxproj | 1 + ...Terminal.Settings.ModelLib.vcxproj.filters | 1 + .../TerminalSettingsModel/NewTabMenu.cpp | 4 +- .../SettingsWriteNotifier.h | 62 ++++++++++++++ src/cascadia/TerminalSettingsModel/Theme.cpp | 20 +++++ src/cascadia/TerminalSettingsModel/Theme.h | 6 +- .../JsonManagerTests.cpp | 78 +++++++++++++++--- 16 files changed, 224 insertions(+), 93 deletions(-) create mode 100644 src/cascadia/TerminalSettingsModel/SettingsWriteNotifier.h diff --git a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp index 48f21b543c..0f6ce866ff 100644 --- a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp @@ -95,6 +95,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // there are some other cases as well Model::ActionAndArgs newActionAndArgs{ actionEnum, emptyArgs }; _command.ActionAndArgs(newActionAndArgs); + get_self(actionsPageVM)->NotifyUserActionEdited(); if (_IsNewCommand) { actionsPageVM.RegenerateCommandID(_command); @@ -129,6 +130,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation if (_command.Name() != newName) { _command.Name(newName); + if (const auto actionsPageVM{ _actionsPageVM.get() }) + { + get_self(actionsPageVM)->NotifyUserActionEdited(); + } _NotifyChanges(L"DisplayName", L"DisplayNameAndKeyChordAutomationPropName"); _cachedDisplayName.clear(); } @@ -309,6 +314,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation actionArgsVM.WrapperValueChanged([weakThis = get_weak()](const IInspectable& /*sender*/, const IInspectable& /*args*/) { if (auto weak = weakThis.get()) { + if (const auto actionsPageVM{ weak->_actionsPageVM.get() }) + { + get_self(actionsPageVM)->NotifyUserActionEdited(); + } // for new commands, make sure we generate a new ID every time any arg value changes if (weak->_IsNewCommand) { @@ -1376,6 +1385,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation _Settings.UpdateCommandID(command, {}); } + void ActionsViewModel::NotifyUserActionEdited() + { + if (_Settings) + { + if (const auto actionMap{ _Settings.ActionMap() }) + { + actionMap.NotifyWriteSettings(); + } + } + } + void ActionsViewModel::_CmdVMEditRequestedHandler(const Editor::CommandViewModel& senderVM, const IInspectable& /*args*/) { CurrentCommand(senderVM); diff --git a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.h b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.h index 8ddada5d7e..8488be3dd3 100644 --- a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.h +++ b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.h @@ -268,6 +268,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void AttemptAddOrModifyKeyChord(const Editor::KeyChordViewModel& senderVM, winrt::hstring commandID, const Control::KeyChord& newKeys, const Control::KeyChord& oldKeys); void AddCopiedCommand(const Model::Command& newCommand); void RegenerateCommandID(const Model::Command& command); + void NotifyUserActionEdited(); Windows::Foundation::Collections::IMap AvailableShortcutActionsAndNames(); Windows::Foundation::Collections::IMap NameToActionMap(); diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index eba0766789..e2d68914a1 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -713,7 +713,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (!name.empty()) { _NestedCommands.emplace(name, cmd); - _NotifyWriteSettings(); + NotifyWriteSettings(); } return; } @@ -722,7 +722,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (cmdImpl->IterateOn() != ExpandCommandType::None) { _IterableCommands.emplace_back(cmd); - _NotifyWriteSettings(); + NotifyWriteSettings(); return; } @@ -732,7 +732,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _TryUpdateActionMap(cmd); _TryUpdateKeyChord(cmd, keys); - _NotifyWriteSettings(); + NotifyWriteSettings(); } // Method Description: @@ -974,7 +974,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _KeyMap.insert_or_assign(oldKeys, L""); } - _NotifyWriteSettings(); + NotifyWriteSettings(); return true; } @@ -999,7 +999,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // set to unbound in this layer _KeyMap.emplace(keys, L""); } - _NotifyWriteSettings(); + NotifyWriteSettings(); } void ActionMap::AddKeyBinding(Control::KeyChord keys, const winrt::hstring& cmdID) @@ -1007,7 +1007,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _KeyMap.insert_or_assign(keys, cmdID); _changeLog.emplace(KeysKey); _RefreshKeyBindingCaches(); - _NotifyWriteSettings(); + NotifyWriteSettings(); } // Method Description: @@ -1030,7 +1030,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { _ActionMap.erase(cmdID); _RefreshKeyBindingCaches(); - _NotifyWriteSettings(); + NotifyWriteSettings(); } // This is a helper to aid in sorting commands by their `Name`s, alphabetically. @@ -1250,7 +1250,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _KeyMap.erase(keys); _KeyMap.emplace(keys, newID); } - _NotifyWriteSettings(); + NotifyWriteSettings(); } _RefreshKeyBindingCaches(); } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.idl b/src/cascadia/TerminalSettingsModel/ActionMap.idl index 097275def9..aceebaa82a 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.idl +++ b/src/cascadia/TerminalSettingsModel/ActionMap.idl @@ -45,5 +45,6 @@ namespace Microsoft.Terminal.Settings.Model void AddKeyBinding(Microsoft.Terminal.Control.KeyChord keys, String cmdID); void RegisterKeyBinding(Microsoft.Terminal.Control.KeyChord keys, ActionAndArgs action); void AddSendInputAction(String name, String input, Microsoft.Terminal.Control.KeyChord keys); + void NotifyWriteSettings(); } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index d399b46de3..c1725f6292 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -1540,6 +1540,14 @@ void CascadiaSettings::_installWriteSink() { winrt::get_self(am)->SetWriteSettingsSink(_writeSink); } + for (const auto& entry : _globals->ColorSchemes()) + { + winrt::get_self(entry.Value())->SetWriteSettingsSink(_writeSink); + } + for (const auto& entry : _globals->Themes()) + { + winrt::get_self(entry.Value())->SetWriteSettingsSink(_writeSink); + } } installForProfile(_baseLayerProfile.get()); diff --git a/src/cascadia/TerminalSettingsModel/ColorScheme.cpp b/src/cascadia/TerminalSettingsModel/ColorScheme.cpp index 8f98a45e4b..9e6af09e52 100644 --- a/src/cascadia/TerminalSettingsModel/ColorScheme.cpp +++ b/src/cascadia/TerminalSettingsModel/ColorScheme.cpp @@ -209,6 +209,7 @@ winrt::hstring ColorScheme::Name() const void ColorScheme::Name(const winrt::hstring& value) { JsonUtils::SetValueForKeyPreservingComments(_json, NameKey, value); + NotifyWriteSettings(); } // Reads a color from _json, falling back to defaultValue when the key is absent. @@ -223,6 +224,7 @@ winrt::Microsoft::Terminal::Core::Color ColorScheme::_getColor(std::string_view void ColorScheme::_setColor(std::string_view key, const Core::Color& value) { JsonUtils::SetValueForKeyPreservingComments(_json, key, value); + NotifyWriteSettings(); } #define GEN_NAMED_COLOR_ACCESSOR(name, jsonKey, defaultVal) \ diff --git a/src/cascadia/TerminalSettingsModel/ColorScheme.h b/src/cascadia/TerminalSettingsModel/ColorScheme.h index 5106874ea6..7d2cb52502 100644 --- a/src/cascadia/TerminalSettingsModel/ColorScheme.h +++ b/src/cascadia/TerminalSettingsModel/ColorScheme.h @@ -19,12 +19,13 @@ Author(s): #include "../../inc/conattrs.hpp" #include "DefaultSettings.h" #include "JsonUtils.h" +#include "SettingsWriteNotifier.h" #include "ColorScheme.g.h" namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - struct ColorScheme : ColorSchemeT + struct ColorScheme : ColorSchemeT, WriteNotifiable { // A ColorScheme constructed with uninitialized_t // leaves _json empty to be populated lazily. diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 5d151e0089..555c7f09e9 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -277,16 +277,14 @@ void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const OriginTa void GlobalAppSettings::AddColorScheme(const Model::ColorScheme& scheme) { _colorSchemes.Insert(scheme.Name(), scheme); - // Coarse-grained auto-save coverage (GH#12424): color schemes aren't - // IInheritable, so add/remove is notified here at the collection level. (No-op - // until this tree is the live, bound tree.) - _NotifyWriteSettings(); + winrt::get_self(scheme)->SetWriteSettingsSink(_writeSettingsSink); + NotifyWriteSettings(); } void GlobalAppSettings::RemoveColorScheme(hstring schemeName) { _colorSchemes.TryRemove(schemeName); - _NotifyWriteSettings(); + NotifyWriteSettings(); } winrt::Microsoft::Terminal::Settings::Model::ColorScheme GlobalAppSettings::DuplicateColorScheme(const Model::ColorScheme& source) @@ -460,9 +458,8 @@ winrt::Microsoft::Terminal::Settings::Model::Theme GlobalAppSettings::CurrentThe void GlobalAppSettings::AddTheme(const Model::Theme& theme) { _themes.Insert(theme.Name(), theme); - // Coarse-grained auto-save coverage (GH#12424): themes aren't IInheritable, - // so add is notified here at the collection level. (No-op until bound.) - _NotifyWriteSettings(); + winrt::get_self(theme)->SetWriteSettingsSink(_writeSettingsSink); + NotifyWriteSettings(); } winrt::Windows::Foundation::Collections::IMapView GlobalAppSettings::Themes() noexcept diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index dcd7e97b67..ca365e1835 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -19,38 +19,12 @@ Author(s): #include "JsonUtils.h" #include "JsonSyncCollections.h" +#include "SettingsWriteNotifier.h" namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - // A late-bound, shared "request auto-save" notifier (GH#12424). One instance - // is created by CascadiaSettings and installed (by shared_ptr) on every - // object in the *live* settings tree. When a setting is mutated, the object - // calls Notify(); CascadiaSettings binds the actual handler after a - // successful load via SetHandler(). Until a handler is bound, Notify() is a - // no-op. The notifier is deliberately NOT propagated by Copy()/clone paths, - // so editor clones (and the defaults fallback) never auto-save the user's - // settings.json. - struct SettingsWriteNotifier - { - void Notify() const - { - if (_handler) - { - _handler(); - } - } - - void SetHandler(std::function handler) - { - _handler = std::move(handler); - } - - private: - std::function _handler; - }; - template - struct IInheritable + struct IInheritable : WriteNotifiable { public: // Method Description: @@ -94,33 +68,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return _parents; } - // A shared "request auto-save" sink. The owning CascadiaSettings - // creates one notifier and installs it on every object in the *live* - // tree. When a setting on this layer is mutated, _NotifyWriteSettings() - // invokes the notifier, which requests a debounced auto-save. The notifier - // is intentionally NOT copied by Copy()/clone paths, so editor clones (and - // the defaults fallback) never auto-save the user's settings.json. - using WriteSettingsSink = std::shared_ptr; - void SetWriteSettingsSink(const WriteSettingsSink& sink) - { - _writeSettingsSink = sink; - } + // The shared "request auto-save" sink (SetWriteSettingsSink, + // _writeSettingsSink, NotifyWriteSettings) is inherited from + // WriteNotifiable. The owning CascadiaSettings installs one shared sink on + // every object in the *live* tree; mutators call NotifyWriteSettings(). + // The sink is intentionally NOT copied by Copy()/clone paths, so editor + // clones (and the defaults fallback) never auto-save the user's settings. protected: std::vector> _parents{}; - WriteSettingsSink _writeSettingsSink{}; - - // Invoked by setters/clears to request an auto-save of the owning tree. - // No-op until a sink is installed (i.e. only on the committed live tree). - void _NotifyWriteSettings() const - { - if (_writeSettingsSink) - { - _writeSettingsSink->Notify(); - } - } - // Method Description: // - Actions to be performed after a child was created. Generally used to set // any extraneous data from the parent into the child. @@ -204,7 +161,7 @@ public: void Clear##name() \ { \ _json.removeMember(JsonKey(jsonKey)); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } \ \ private: \ @@ -273,7 +230,7 @@ public: \ } \ ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey( \ _json, jsonKey, value); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } // JSON-backed inheritable setting. @@ -295,7 +252,7 @@ public: \ { \ ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey( \ _json, jsonKey, value); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } // JSON-backed inheritable setting with change logging. @@ -340,7 +297,7 @@ public: /* explicitly set to null (not the same as clearing) */ \ _json[JsonKey(jsonKey)] = Json::nullValue; \ } \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } // ============================================================================= @@ -466,7 +423,7 @@ public: _##name = value; \ ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey(_json, jsonKey, value); \ _logSettingSet(jsonKey); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } \ \ /* Dual-clear: backing field + _json key. Both must clear so auto-save doesn't */ \ @@ -476,7 +433,7 @@ public: _##name = std::nullopt; \ _json.removeMember(JsonKey(jsonKey)); \ _logSettingSet(jsonKey); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } // IMediaResource vector setting (BellSound): needs both IMediaResource @@ -516,7 +473,7 @@ public: .ToJson(current); \ strong->_json[JsonKey(jsonKey)] = std::move(temp); \ strong->_logSettingSet(jsonKey); \ - strong->_NotifyWriteSettings(); \ + strong->NotifyWriteSettings(); \ }); \ } \ return *val; \ @@ -528,7 +485,7 @@ public: _##name = value; \ ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey(_json, jsonKey, value); \ _logSettingSet(jsonKey); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } \ \ /* Dual-clear: backing field + _json key. */ \ @@ -537,7 +494,7 @@ public: _##name = std::nullopt; \ _json.removeMember(JsonKey(jsonKey)); \ _logSettingSet(jsonKey); \ - _NotifyWriteSettings(); \ + NotifyWriteSettings(); \ } // JSON-backed collection settings (IVector / IMap) that callers mutate @@ -568,7 +525,7 @@ public: auto temp = ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait{}.ToJson(current); \ strong->_json[JsonKey(jsonKey)] = std::move(temp); \ strong->_logSettingSet(jsonKey); \ - strong->_NotifyWriteSettings(); \ + strong->NotifyWriteSettings(); \ }); \ } \ \ @@ -595,7 +552,7 @@ public: auto temp = ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait{}.ToJson(current); \ strong->_json[JsonKey(jsonKey)] = std::move(temp); \ strong->_logSettingSet(jsonKey); \ - strong->_NotifyWriteSettings(); \ + strong->NotifyWriteSettings(); \ }); \ } \ \ diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index f9265dfa34..820d754abe 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -90,6 +90,7 @@ JsonManager.idl + diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj.filters b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj.filters index 14de9bea78..f4c7895220 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj.filters +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj.filters @@ -82,6 +82,7 @@ json + diff --git a/src/cascadia/TerminalSettingsModel/NewTabMenu.cpp b/src/cascadia/TerminalSettingsModel/NewTabMenu.cpp index b66b0923d2..7b437cbf4b 100644 --- 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 }; - _NotifyWriteSettings(); + NotifyWriteSettings(); } void NewTabMenu::ReplaceAll(const IVector& entries) @@ -377,7 +377,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } _json = std::move(arr); _emitChangeLog(); - _NotifyWriteSettings(); + NotifyWriteSettings(); } void NewTabMenu::_emitChangeLog() diff --git a/src/cascadia/TerminalSettingsModel/SettingsWriteNotifier.h b/src/cascadia/TerminalSettingsModel/SettingsWriteNotifier.h new file mode 100644 index 0000000000..579e66eb51 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/SettingsWriteNotifier.h @@ -0,0 +1,62 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- SettingsWriteNotifier.h + +Abstract: +- The late-bound "request auto-save" sink shared by every settings-model object + in the live tree. + +Author(s): +- Carlos Zamora - June 2026 + +--*/ +#pragma once + +#include +#include + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + struct SettingsWriteNotifier + { + void Notify() const + { + if (_handler) + { + _handler(); + } + } + + void SetHandler(std::function handler) + { + _handler = std::move(handler); + } + + private: + std::function _handler; + }; + + struct WriteNotifiable + { + public: + using WriteSettingsSink = std::shared_ptr; + void SetWriteSettingsSink(const WriteSettingsSink& sink) + { + _writeSettingsSink = sink; + } + + void NotifyWriteSettings() const + { + if (_writeSettingsSink) + { + _writeSettingsSink->Notify(); + } + } + + protected: + WriteSettingsSink _writeSettingsSink{}; + }; +} diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index fa3023a1bf..10a942681a 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -248,6 +248,26 @@ winrt::com_ptr Theme::Copy() const return theme; } +void Theme::SetWriteSettingsSink(const WriteNotifiable::WriteSettingsSink& sink) +{ + if (_Window) + { + winrt::get_self(_Window)->SetWriteSettingsSink(sink); + } + if (_Settings) + { + winrt::get_self(_Settings)->SetWriteSettingsSink(sink); + } + if (_TabRow) + { + winrt::get_self(_TabRow)->SetWriteSettingsSink(sink); + } + if (_Tab) + { + winrt::get_self(_Tab)->SetWriteSettingsSink(sink); + } +} + // Method Description: // - Create a new instance of this class from a serialized JsonObject. // Arguments: diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h index 7c3a9371cc..2b0b2ea258 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.h +++ b/src/cascadia/TerminalSettingsModel/Theme.h @@ -27,6 +27,7 @@ Author(s): #include "Theme.g.h" #include "JsonUtils.h" +#include "SettingsWriteNotifier.h" namespace winrt::Microsoft::Terminal::Settings::Model::implementation { @@ -85,6 +86,7 @@ public: void name(const type& value) \ { \ ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey(_json, jsonKey, value); \ + NotifyWriteSettings(); \ } \ bool Has##name() const \ { \ @@ -93,10 +95,11 @@ public: void Clear##name() \ { \ _json.removeMember(jsonKey); \ + NotifyWriteSettings(); \ } #define THEME_OBJECT(className, macro) \ - struct className : className##T \ + struct className : className##T, WriteNotifiable \ { \ winrt::com_ptr Copy(); \ Json::Value ToJson(); \ @@ -126,6 +129,7 @@ public: static com_ptr FromJson(const Json::Value& json); Json::Value ToJson() const; void LogSettingChanges(std::set& changes, const std::string_view& context); + void SetWriteSettingsSink(const WriteNotifiable::WriteSettingsSink& sink); winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept; diff --git a/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp b/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp index 83c3072005..6248d69386 100644 --- a/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp @@ -5,6 +5,7 @@ #include "../TerminalSettingsModel/CascadiaSettings.h" #include "../TerminalSettingsModel/ColorScheme.h" +#include "../TerminalSettingsModel/Theme.h" #include "../TerminalSettingsModel/ActionMap.h" #include "../TerminalSettingsModel/FileUtils.h" #include "JsonTestClass.h" @@ -30,6 +31,8 @@ namespace SettingsModelUnitTests TEST_METHOD(WriteHandlerFiresOnSetterAndClear); TEST_METHOD(ClonedSettingsDoNotFireWriteHandler); TEST_METHOD(CollectionEditsFireWriteHandler); + TEST_METHOD(ColorSchemePerPropertyEditsFireWriteHandler); + TEST_METHOD(ThemeSubObjectEditsFireWriteHandler); TEST_METHOD(ActionMapEditsFireWriteHandler); private: @@ -164,9 +167,6 @@ namespace SettingsModelUnitTests void JsonManagerTests::CollectionEditsFireWriteHandler() { - // Color schemes / themes aren't IInheritable, so per-property edits aren't - // covered, but add/remove is wired at the GlobalAppSettings collection - // level. Verify that coarse-grained coverage requests a save. static constexpr std::string_view settingsJson{ R"({ "profiles": { "list": [ @@ -191,11 +191,67 @@ namespace SettingsModelUnitTests VERIFY_ARE_EQUAL(2, writeRequests); } + void JsonManagerTests::ColorSchemePerPropertyEditsFireWriteHandler() + { + static constexpr std::string_view settingsJson{ R"({ + "profiles": { + "list": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}" + } + ] + } + })" }; + + const auto settings = winrt::make_self(settingsJson); + + auto writeRequests = 0; + settings->SetWriteHandler([&]() { ++writeRequests; }); + + // Adding the scheme forwards the sink to it; the add itself requests a save. + const auto scheme = winrt::make(winrt::hstring{ L"My Scheme" }); + settings->GlobalSettings().AddColorScheme(scheme); + VERIFY_ARE_EQUAL(1, writeRequests); + + // Each per-property edit requests a save. + scheme.Foreground(til::color{ 0x10, 0x20, 0x30 }); + VERIFY_ARE_EQUAL(2, writeRequests); + + scheme.SetColorTableEntry(0, til::color{ 0x40, 0x50, 0x60 }); + VERIFY_ARE_EQUAL(3, writeRequests); + + scheme.Name(L"Renamed Scheme"); + VERIFY_ARE_EQUAL(4, writeRequests); + } + + void JsonManagerTests::ThemeSubObjectEditsFireWriteHandler() + { + static constexpr std::string_view themeJson{ R"({ + "name": "MyTheme", + "window": { "useMica": false } + })" }; + + const auto theme = implementation::Theme::FromJson(VerifyParseSucceeded(themeJson)); + VERIFY_IS_NOT_NULL(theme); + + auto writeRequests = 0; + const auto sink = std::make_shared(); + sink->SetHandler([&]() { ++writeRequests; }); + theme->SetWriteSettingsSink(sink); + + const auto window = theme->Window(); + VERIFY_IS_NOT_NULL(window); + + winrt::get_self(window)->UseMica(true); + VERIFY_ARE_EQUAL(1, writeRequests); + + winrt::get_self(window)->ClearUseMica(); + VERIFY_ARE_EQUAL(2, writeRequests); + } + void JsonManagerTests::ActionMapEditsFireWriteHandler() { - // ActionMap isn't JSON-backed, but its mutators request a save directly and - // it's registered with the write sink, so user action/keybinding edits must - // request an auto-save. static constexpr std::string_view settingsJson{ R"({ "profiles": { "list": [ @@ -246,10 +302,10 @@ namespace SettingsModelUnitTests actionMap.DeleteUserCommand(L"Test.NewTab"); VERIFY_ARE_EQUAL(5, writeRequests); - // The editor works on a deep clone, which must never auto-save the live - // file: a clone's ActionMap edit must NOT invoke the handler. - const auto clone = settings->Copy(); - clone.GlobalSettings().ActionMap().AddKeyBinding(ctrlT, L"Test.NewTab"); - VERIFY_ARE_EQUAL(5, writeRequests); + // An in-place editor edit (rename/action/arg change mutates the Command + // directly, bypassing the mutators above) is signaled via the projected + // NotifyWriteSettings and must request a save. + actionMap.NotifyWriteSettings(); + VERIFY_ARE_EQUAL(6, writeRequests); } }