From 619c48023be0b3c0b85b1efbb10a9bcd84ef2a7e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 4 Jun 2026 18:16:27 -0700 Subject: [PATCH] Iterate on JsonManager (needs review) --- .../TerminalApp/AppActionHandlers.cpp | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 4 +- .../TerminalSettingsEditor/Compatibility.cpp | 2 +- .../TerminalSettingsEditor/MainPage.cpp | 13 +- .../TerminalSettingsModel/CascadiaSettings.h | 15 +- .../CascadiaSettings.idl | 2 - .../CascadiaSettingsSerialization.cpp | 88 +++------ .../GlobalAppSettings.cpp | 8 + .../TerminalSettingsModel/IInheritable.h | 43 ++++- .../TerminalSettingsModel/JsonManager.cpp | 170 +++++++++++------- .../TerminalSettingsModel/JsonManager.h | 40 ++++- .../TerminalSettingsModel/JsonManager.idl | 14 +- .../JsonManagerTests.cpp | 31 ++++ 13 files changed, 264 insertions(+), 168 deletions(-) diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index 7e0edef7bd..f7a45bd812 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -1318,7 +1318,7 @@ namespace winrt::TerminalApp::implementation keyChord = KeyChordSerialization::FromString(winrt::to_hstring(realArgs.KeyChord())); } _settings.GlobalSettings().ActionMap().AddSendInputAction(realArgs.Name(), commandLine, keyChord); - _settings.WriteSettingsToDisk(); + JsonManager::WriteSettings(_settings); ActionSaved(commandLine, realArgs.Name(), realArgs.KeyChord()); } catch (const winrt::hresult_error& ex) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 31c4bba516..86a825f019 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -951,7 +951,9 @@ namespace winrt::TerminalApp::implementation if (result == ContentDialogResult::Primary && checkbox.IsChecked().Value()) { _settings.GlobalSettings().ConfirmOnClose(ConfirmOnClose::Never); - _settings.WriteSettingsToDisk(); + // Persisted via auto-save: the setter fires the write sink, which + // updates the JsonManager baseline and avoids a redundant write + + // spurious reload (GH#12424). A pending write is flushed on exit. } } diff --git a/src/cascadia/TerminalSettingsEditor/Compatibility.cpp b/src/cascadia/TerminalSettingsEditor/Compatibility.cpp index 82e39190c5..1ed671fcfd 100644 --- a/src/cascadia/TerminalSettingsEditor/Compatibility.cpp +++ b/src/cascadia/TerminalSettingsEditor/Compatibility.cpp @@ -45,7 +45,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); - _settings.ResetToDefaultSettings(); + JsonManager::ResetToDefaultSettings(); } Compatibility::Compatibility() diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index 2f3cd75db0..dd2d857340 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -861,9 +861,18 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/) { _settingsClone.LogSettingChanges(false); - if (!_settingsClone.WriteSettingsToDisk()) + if (JsonManager::WriteSettings(_settingsClone).empty()) { - ShowLoadWarningsDialog.raise(*this, _settingsClone.Warnings()); + // The write failed. Surface the clone's existing warnings plus an + // explicit "failed to write" warning (the old WriteSettingsToDisk + // appended this internally). + auto warnings{ winrt::single_threaded_vector() }; + for (auto&& w : _settingsClone.Warnings()) + { + warnings.Append(w); + } + warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings); + ShowLoadWarningsDialog.raise(*this, warnings.GetView()); } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index ec1045fe10..97eee0b39e 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -143,6 +143,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation struct CascadiaSettings : CascadiaSettingsT { + // JsonManager performs settings.json persistence (GH#12424) and reads the + // cached _currentDefaultTerminal directly to persist the default-terminal + // choice without triggering an expensive defterm refresh. + friend struct JsonManager; + public: static Model::CascadiaSettings LoadDefaults(); static Model::CascadiaSettings LoadAll(); @@ -168,8 +173,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::ActionMap ActionMap() const noexcept; winrt::Windows::Foundation::Collections::IVectorView Extensions(); void ResetApplicationState() const; - void ResetToDefaultSettings(); - bool WriteSettingsToDisk(); void SetWriteHandler(std::function handler); Json::Value ToJson() const; @@ -203,16 +206,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: static const std::filesystem::path& _settingsPath(); static const std::filesystem::path& _releaseSettingsPath(); - static winrt::hstring _calculateHash(std::string_view settings, const FILETIME& lastWriteTime); winrt::com_ptr _createNewProfile(const std::wstring_view& name) const; Model::Profile _getProfileForCommandLine(const winrt::hstring& commandLine) const; void _refreshDefaultTerminals(); - void _writeSettingsToDisk(std::string_view contents); - // Auto-save (GH#12424): the shared "request auto-save" sink installed on - // every inheritable object in this live tree. Empty until SetWriteHandler. - std::shared_ptr> _writeSink; + // Auto-save (GH#12424): the shared "request auto-save" notifier installed + // on every inheritable object in this live tree. Empty until SetWriteHandler. + std::shared_ptr _writeSink; void _installWriteSink(); void _resolveDefaultProfile() const; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl index cf6e258ddb..3f71294015 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -30,8 +30,6 @@ namespace Microsoft.Terminal.Settings.Model CascadiaSettings Copy(); void ResetApplicationState(); - void ResetToDefaultSettings(); - Boolean WriteSettingsToDisk(); void LogSettingChanges(Boolean isJsonLoad); String Hash { get; }; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index bed230825d..2545e3b21c 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -20,6 +20,7 @@ #include "ApplicationState.h" #include "DefaultTerminal.h" #include "FileUtils.h" +#include "JsonManager.h" #include "ProfileEntry.h" #include "FolderEntry.h" @@ -1284,13 +1285,23 @@ try // settings string back to the file. if (mustWriteToDisk) { - settings->WriteSettingsToDisk(); + // JsonManager::WriteSettings (GH#12424) force-writes and returns the new + // hash (empty on failure); capture it so reload-detection has an accurate + // baseline. + const auto hash = JsonManager::WriteSettings(settings.as()); + if (hash.empty()) + { + settings->_warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings); + } + else + { + settings->_hash = hash; + } } else { // lastWriteTime is only valid if mustWriteToDisk is false. - // Additionally WriteSettingsToDisk() updates the _hash for us already. - settings->_hash = _calculateHash(settingsString, lastWriteTime); + settings->_hash = CalculateSettingsHash(settingsString, lastWriteTime); } settings->_researchOnLoad(); @@ -1496,7 +1507,7 @@ CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) : // and the defaults fallback never auto-save. void CascadiaSettings::_installWriteSink() { - _writeSink = std::make_shared>(); + _writeSink = std::make_shared(); const auto installForProfile = [&](implementation::Profile* prof) { if (!prof) @@ -1532,13 +1543,22 @@ void CascadiaSettings::_installWriteSink() { installForProfile(winrt::get_self(p)); } + + // TODO CARLOS: Known gaps (GH#12424): ColorScheme and Theme are not IInheritable / not + // JSON-backed, so per-property in-place edits (e.g. SetColorTableEntry, + // individual theme colors) do NOT trigger auto-save. ActionMap is IInheritable + // but not JSON-backed, so individual action edits don't either. Add/remove of + // color schemes and themes IS covered via the GlobalAppSettings collection + // mutators (AddColorScheme/RemoveColorScheme/DuplicateColorScheme/AddTheme). + // Full per-property coverage waits for those types becoming JSON-backed and is + // only required once the settings editor relies on auto-save. } void CascadiaSettings::SetWriteHandler(std::function handler) { if (_writeSink) { - *_writeSink = std::move(handler); + _writeSink->SetHandler(std::move(handler)); } } @@ -1566,12 +1586,6 @@ const std::filesystem::path& CascadiaSettings::_releaseSettingsPath() return path; } -// Returns a has (approximately) uniquely identifying the settings.json contents on disk. -winrt::hstring CascadiaSettings::_calculateHash(std::string_view settings, const FILETIME& lastWriteTime) -{ - return CalculateSettingsHash(settings, lastWriteTime); -} - // This returns something akin to %LOCALAPPDATA%\Packages\WindowsTerminalDev_8wekyb3d8bbwe\LocalState // just like SettingsPath(), but without the trailing \settings.json. winrt::hstring CascadiaSettings::SettingsDirectory() @@ -1628,58 +1642,6 @@ void CascadiaSettings::ResetApplicationState() const state.Flush(); } -void CascadiaSettings::ResetToDefaultSettings() -{ - ApplicationState::SharedInstance().Reset(); - _writeSettingsToDisk(LoadStringResource(IDR_USER_DEFAULTS)); -} - -// Method Description: -// - Write the current state of CascadiaSettings to our settings file -// - Create a backup file with the current contents, if one does not exist -// - Persists the default terminal handler choice to the registry -// Arguments: -// - -// Return Value: -// - -bool CascadiaSettings::WriteSettingsToDisk() -{ - // write current settings to current settings file - Json::StreamWriterBuilder wbuilder; - wbuilder.settings_["enableYAMLCompatibility"] = true; // suppress spaces around colons - wbuilder.settings_["indentation"] = " "; - wbuilder.settings_["precision"] = 6; // prevent values like 1.1000000000000001 - - try - { - _writeSettingsToDisk(Json::writeString(wbuilder, ToJson())); - } - catch (...) - { - LOG_CAUGHT_EXCEPTION(); - _warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings); - return false; - } - return true; -} - -void CascadiaSettings::_writeSettingsToDisk(std::string_view contents) -{ - FILETIME lastWriteTime{}; - // Auto-save (GH#12424): write atomically via a process-unique temp file and - // keep a best-effort "settings.json.bak" backup of the prior contents. - WriteSettingsFile(_settingsPath(), contents, /*makeBackup*/ true, &lastWriteTime); - - _hash = _calculateHash(contents, lastWriteTime); - - // Persists the default terminal choice - // GH#10003 - Only do this if _currentDefaultTerminal was actually initialized. - if (_currentDefaultTerminal) - { - DefaultTerminal::Current(_currentDefaultTerminal); - } -} - #ifndef NDEBUG [[maybe_unused]] static std::string _getDevPathToSchema() { diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 7217295e9a..5d151e0089 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -277,11 +277,16 @@ 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(); } void GlobalAppSettings::RemoveColorScheme(hstring schemeName) { _colorSchemes.TryRemove(schemeName); + _NotifyWriteSettings(); } winrt::Microsoft::Terminal::Settings::Model::ColorScheme GlobalAppSettings::DuplicateColorScheme(const Model::ColorScheme& source) @@ -455,6 +460,9 @@ 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::Windows::Foundation::Collections::IMapView GlobalAppSettings::Themes() noexcept diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index d4f6fef5e5..dcd7e97b67 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -22,6 +22,33 @@ Author(s): 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 { @@ -68,12 +95,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } // A shared "request auto-save" sink. The owning CascadiaSettings - // creates one sink and installs it on every object in the *live* tree. - // When a setting on this layer is mutated, _NotifyWriteSettings() invokes - // the sink, which requests a debounced auto-save. The sink 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>; + // 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; @@ -88,9 +115,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // No-op until a sink is installed (i.e. only on the committed live tree). void _NotifyWriteSettings() const { - if (_writeSettingsSink && *_writeSettingsSink) + if (_writeSettingsSink) { - (*_writeSettingsSink)(); + _writeSettingsSink->Notify(); } } diff --git a/src/cascadia/TerminalSettingsModel/JsonManager.cpp b/src/cascadia/TerminalSettingsModel/JsonManager.cpp index 57234d879f..a0b6b30252 100644 --- a/src/cascadia/TerminalSettingsModel/JsonManager.cpp +++ b/src/cascadia/TerminalSettingsModel/JsonManager.cpp @@ -5,6 +5,9 @@ #include "JsonManager.h" #include "CascadiaSettings.h" #include "FileUtils.h" +#include "ApplicationState.h" +#include "DefaultTerminal.h" +#include "resource.h" #include @@ -31,10 +34,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation JsonManager::~JsonManager() { - // Tear down the watcher before the rest of *this is destroyed. - // _writeThrottler (last-declared member) is destroyed first, cancelling - // any pending callback. + // Stop watching first so the flush's write below can't trigger a + // self-write callback during teardown. Stop(); + + // Flush any pending auto-save so a debounced write isn't lost on shutdown + // (e.g. a setting changed moments before the app exits). The flush runs + // _write(), which only touches the captured byte snapshot -- not the tree. + _writeThrottler.flush(); } // Begins watching the settings directory for external changes. @@ -70,18 +77,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // loaded tree (never a failed load or the defaults fallback). void JsonManager::SetLiveSettings(const Model::CascadiaSettings& settings) { + // Detach the previous tree's write handler so a stale tree still retained + // elsewhere can no longer schedule saves against us. + if (auto old = _liveSettings.get()) + { + winrt::get_self(old)->SetWriteHandler(nullptr); + } + { std::scoped_lock lock{ _stateMutex }; - ++_generation; + _generation.bump(); _pendingSnapshot.clear(); - _autoSaveEnabled = static_cast(settings); _expectedDiskHash = settings ? settings.Hash() : winrt::hstring{}; } - _liveSettings = settings ? winrt::make_weak(settings) : winrt::weak_ref{ nullptr }; - if (settings) { + _liveSettings = winrt::make_weak(settings); + auto self = winrt::get_self(settings); auto weakThis = get_weak(); self->SetWriteHandler([weakThis]() { @@ -91,20 +104,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } }); } + else + { + _liveSettings = nullptr; + } } // Unbinds the live tree and disables auto-save (e.g. on load failure or - // when falling back to the defaults settings object). + // when falling back to the defaults settings object). Identical to binding a + // null tree. void JsonManager::ClearLiveSettings() { - { - std::scoped_lock lock{ _stateMutex }; - ++_generation; - _pendingSnapshot.clear(); - _autoSaveEnabled = false; - _expectedDiskHash = {}; - } - _liveSettings = winrt::weak_ref{ nullptr }; + SetLiveSettings(nullptr); } // Captures a snapshot of the bound tree (on the calling/owner thread) and @@ -112,22 +123,19 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // thread, keeps all model-tree traversal on the owner thread. void JsonManager::RequestSave() { - uint64_t generation{}; - { - std::scoped_lock lock{ _stateMutex }; - if (!_autoSaveEnabled) - { - return; - } - generation = _generation; - } - + // A bound (non-null) tree is exactly what enables auto-save. const auto live = _liveSettings.get(); if (!live) { return; } + til::generation_t generation; + { + std::scoped_lock lock{ _stateMutex }; + generation = _generation; + } + std::string snapshot; try { @@ -153,37 +161,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _writeThrottler(); } - // Synchronously writes the bound tree to disk. Used by callers that need the - // write to complete before returning (rather than the debounced path). - bool JsonManager::Save() - { - { - std::scoped_lock lock{ _stateMutex }; - if (!_autoSaveEnabled) - { - return false; - } - } - - const auto live = _liveSettings.get(); - if (!live) - { - return false; - } - - std::string snapshot; - try - { - snapshot = _serialize(live); - } - catch (...) - { - LOG_CAUGHT_EXCEPTION(); - return false; - } - return _persistSnapshot(snapshot); - } - // Debounce callback (timer thread): writes the captured snapshot. void JsonManager::_write() noexcept { @@ -227,22 +204,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation FILETIME diskTime{}; const auto diskContent = til::io::read_file_as_utf8_string_if_exists(path, false, &diskTime); - if (!diskContent.empty()) + if (diskContent.empty()) + { + // The file was deleted/emptied externally. Treat as a conflict so + // we don't silently recreate it, unless we never had a baseline + // (nothing to lose). + conflict = !_expectedDiskHash.empty(); + } + else { const auto diskHash = CalculateSettingsHash(diskContent, diskTime); - if (!_expectedDiskHash.empty() && diskHash != _expectedDiskHash) - { - // Disk diverged from our baseline, likely due to an external edit. Disk is - // the source of truth, so drop our pending change. - conflict = true; - } + // Disk diverged from our baseline, likely due to an external edit. + // Disk is the source of truth, so drop our pending change. + conflict = !_expectedDiskHash.empty() && diskHash != _expectedDiskHash; } if (!conflict) { - FILETIME newTime{}; - WriteSettingsFile(path, snapshot, /*makeBackup*/ true, &newTime); - _expectedDiskHash = CalculateSettingsHash(snapshot, newTime); + _expectedDiskHash = _writeContent(snapshot); } } @@ -296,8 +275,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation LOG_CAUGHT_EXCEPTION(); } - // Serializes a settings tree to JSON using the same writer settings as - // CascadiaSettings::WriteSettingsToDisk. Must run on the owner thread. + // Serializes a settings tree to JSON. Walks the model tree (ToJson), so it + // must run on the owner thread. std::string JsonManager::_serialize(const Model::CascadiaSettings& settings) { auto self = winrt::get_self(settings); @@ -309,4 +288,57 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return Json::writeString(wbuilder, self->ToJson()); } + + // Low-level write: atomically writes content to settings.json (with a .bak + // backup) and returns the resulting content/time hash. Pure I/O -- it never + // touches the model tree, so it is safe to call from the background timer + // thread (the auto-save path) as well as from the owner thread. + winrt::hstring JsonManager::_writeContent(std::string_view content) + { + const std::filesystem::path path{ std::wstring_view{ CascadiaSettings::SettingsPath() } }; + FILETIME newTime{}; + WriteSettingsFile(path, content, /*makeBackup*/ true, &newTime); + return CalculateSettingsHash(content, newTime); + } + + // Force-writes a tree to disk: serialize + atomic write + .bak + persist the + // default-terminal choice. Returns the new hash (empty on failure). Static -- + // no instance and no auto-save baseline check. Used by load-time fixups, the + // settings-editor clone save, and other explicit savers. Must run on the + // thread that owns the tree (it reads tree state for the defterm persist). + winrt::hstring JsonManager::WriteSettings(const Model::CascadiaSettings& settings) + try + { + if (!settings) + { + return {}; + } + + const auto content = _serialize(settings); + const auto hash = _writeContent(content); + + // Persist the default terminal choice (GH#10003: only when actually + // initialized). Read the cached member directly -- the public getter + // would trigger an expensive defterm refresh. + auto self = winrt::get_self(settings); + if (self->_currentDefaultTerminal) + { + DefaultTerminal::Current(self->_currentDefaultTerminal); + } + + return hash; + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + return {}; + } + + // Resets ApplicationState and writes the user-defaults template to disk. + void JsonManager::ResetToDefaultSettings() + { + ApplicationState::SharedInstance().Reset(); + const std::filesystem::path path{ std::wstring_view{ CascadiaSettings::SettingsPath() } }; + WriteSettingsFile(path, LoadStringResource(IDR_USER_DEFAULTS), /*makeBackup*/ true); + } } diff --git a/src/cascadia/TerminalSettingsModel/JsonManager.h b/src/cascadia/TerminalSettingsModel/JsonManager.h index 86ad0c8179..3254501617 100644 --- a/src/cascadia/TerminalSettingsModel/JsonManager.h +++ b/src/cascadia/TerminalSettingsModel/JsonManager.h @@ -8,12 +8,23 @@ Module Name: Abstract: - Owns settings.json persistence and file-change watching, split out of CascadiaSettings as part of the Auto-Save effort. -- An instance is owned by the app (AppLogic), not a singleton. AppLogic binds - the live settings tree via SetLiveSettings() ONLY after a successful load, - and disables auto-save on load failure / defaults fallback so we never - overwrite a user's broken settings.json with defaults. +- Ownership: an instance is owned by the app (AppLogic), NOT a process singleton. + The decision of "when NOT to save" lives with AppLogic — it binds the live tree + via SetLiveSettings() ONLY after a successful load, and never binds the + LoadDefaults() fallback, so we never overwrite a user's broken settings.json + with defaults. There is one AppLogic per process, so an instance is already + process-scoped; instance ownership also gives clean lifetime + testability that + a singleton holding the live model tree would not. - The bound tree is held weakly and guarded by a generation token so a scheduled write can never run against a superseded tree. +- Threading: the agile weak_ref is touched only on the owner (UI) thread, because + the model *tree* it points at is not thread-safe. Serialization is snapshotted + on the owner thread (RequestSave) and only the resulting immutable byte snapshot + crosses to the background debounce-timer / file-I/O thread. +- The static WriteSettings()/ResetToDefaultSettings() are force-writes callable + without an instance (used by load-time fixups, the editor clone save, and other + explicit savers). The instance auto-save path is the only debounced one and is + the only path that performs the baseline lost-update conflict check. Author(s): - Carlos Zamora - June 2026 @@ -25,6 +36,7 @@ Author(s): #include #include +#include #include #include @@ -42,19 +54,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ClearLiveSettings(); void RequestSave(); - bool Save(); + + // Force-write the given tree to disk (serialize + atomic write + .bak + + // persist default-terminal choice). Returns the new content/time hash, or + // an empty string on failure. Static so it works without an instance: + // used by load-time fixups, the settings-editor clone save, and other + // explicit savers. Does NOT perform the auto-save baseline conflict check. + static winrt::hstring WriteSettings(const Model::CascadiaSettings& settings); + + // Reset ApplicationState and write the user-defaults template to disk. + static void ResetToDefaultSettings(); TYPED_EVENT(SettingsChangedExternally, Model::JsonManager, Windows::Foundation::IInspectable); private: // The live settings tree, bound only after a successful load. Held // weakly so JsonManager never keeps a superseded/failed tree alive. - // Only touched on the owner (UI) thread. + // Only touched on the owner (UI) thread (see Threading note above). A + // bound (non-null) tree is exactly what enables auto-save. winrt::weak_ref _liveSettings{ nullptr }; // Bumped on every (Set|Clear)LiveSettings so a write scheduled against // a previous tree can detect that it is stale and bail out. - uint64_t _generation{ 0 }; + til::generation_t _generation; // The folder watcher over the settings directory. Created in Start(), // torn down in Stop(). Its callback runs on a background thread. @@ -63,12 +85,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // State shared between the owner thread, the debounce timer thread, and // the watcher thread. Guarded by _stateMutex. std::mutex _stateMutex; - bool _autoSaveEnabled{ false }; std::string _pendingSnapshot; // The generation the pending snapshot was captured under. If the live // tree is swapped (reload) before the debounced write fires, the // generation no longer matches and the stale snapshot is dropped. - uint64_t _pendingSnapshotGeneration{ 0 }; + til::generation_t _pendingSnapshotGeneration; // The hash we expect settings.json to currently have on disk. Used to // distinguish our own writes from external edits, and as the baseline // for the lost-update conflict check. @@ -84,6 +105,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _onFileChanged() noexcept; bool _persistSnapshot(const std::string& snapshot) noexcept; static std::string _serialize(const Model::CascadiaSettings& settings); + static winrt::hstring _writeContent(std::string_view content); }; } diff --git a/src/cascadia/TerminalSettingsModel/JsonManager.idl b/src/cascadia/TerminalSettingsModel/JsonManager.idl index 1f6d7a8b75..c1175f159e 100644 --- a/src/cascadia/TerminalSettingsModel/JsonManager.idl +++ b/src/cascadia/TerminalSettingsModel/JsonManager.idl @@ -13,6 +13,15 @@ namespace Microsoft.Terminal.Settings.Model { JsonManager(); + // Force-writes the given tree to disk (serialize + atomic write + .bak). + // Returns the new content/time hash, or an empty string on failure. + // Static so it works without an instance: used by load-time fixups, the + // settings-editor clone save, and other explicit savers. + static String WriteSettings(CascadiaSettings settings); + + // Resets ApplicationState and writes the user-defaults template to disk. + static void ResetToDefaultSettings(); + // Lifecycle. The folder watcher is owned here but explicitly started // and stopped by the app. JsonManager never reloads or touches // UI-bound state itself; it only raises SettingsChangedExternally. @@ -28,11 +37,6 @@ namespace Microsoft.Terminal.Settings.Model // Schedules a debounced auto-save of the bound tree. No-op if unbound. void RequestSave(); - // Synchronously writes the bound tree to disk. Used by load-time - // fixups, which must not route through the debounce. Returns true on - // success. - Boolean Save(); - // Raised when an external (non-self) edit to settings.json is detected // on disk. The app handles marshaling any reload onto the UI thread. event Windows.Foundation.TypedEventHandler SettingsChangedExternally; diff --git a/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp b/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp index 07ea3386c9..06b0529330 100644 --- a/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/JsonManagerTests.cpp @@ -4,6 +4,7 @@ #include "pch.h" #include "../TerminalSettingsModel/CascadiaSettings.h" +#include "../TerminalSettingsModel/ColorScheme.h" #include "../TerminalSettingsModel/FileUtils.h" #include "JsonTestClass.h" @@ -26,6 +27,7 @@ namespace SettingsModelUnitTests TEST_METHOD(WriteSettingsFileCreatesBackup); TEST_METHOD(WriteHandlerFiresOnSetterAndClear); TEST_METHOD(ClonedSettingsDoNotFireWriteHandler); + TEST_METHOD(CollectionEditsFireWriteHandler); private: static std::filesystem::path _tempFile(const wchar_t* name) @@ -156,4 +158,33 @@ namespace SettingsModelUnitTests settings->AllProfiles().GetAt(0).HistorySize(7000); VERIFY_ARE_EQUAL(1, writeRequests); } + + 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": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}" + } + ] + } + })" }; + + const auto settings = winrt::make_self(settingsJson); + + auto writeRequests = 0; + settings->SetWriteHandler([&]() { ++writeRequests; }); + + const auto scheme = winrt::make(winrt::hstring{ L"My Scheme" }); + settings->GlobalSettings().AddColorScheme(scheme); + VERIFY_ARE_EQUAL(1, writeRequests); + + settings->GlobalSettings().RemoveColorScheme(L"My Scheme"); + VERIFY_ARE_EQUAL(2, writeRequests); + } }