Iterate on JsonManager (needs review)

Author: carlos-zamora

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)

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.
             }
         }
 

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()

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<SettingsLoadWarnings>() };
+            for (auto&& w : _settingsClone.Warnings())
+            {
+                warnings.Append(w);
+            }
+            warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings);
+            ShowLoadWarningsDialog.raise(*this, warnings.GetView());
         }
     }
 

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

@@ -143,6 +143,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
     struct CascadiaSettings : CascadiaSettingsT<CascadiaSettings>
     {
+        // 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<Model::ExtensionPackage> Extensions();
         void ResetApplicationState() const;
-        void ResetToDefaultSettings();
-        bool WriteSettingsToDisk();
         void SetWriteHandler(std::function<void()> 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<implementation::Profile> _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<std::function<void()>> _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<implementation::SettingsWriteNotifier> _writeSink;
         void _installWriteSink();
 
         void _resolveDefaultProfile() const;

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; };

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<Model::CascadiaSettings>());
+        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<std::function<void()>>();
+    _writeSink = std::make_shared<implementation::SettingsWriteNotifier>();
 
     const auto installForProfile = [&](implementation::Profile* prof) {
         if (!prof)
@@ -1532,13 +1543,22 @@ void CascadiaSettings::_installWriteSink()
     {
         installForProfile(winrt::get_self<implementation::Profile>(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<void()> 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:
-// - <none>
-// Return Value:
-// - <none>
-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()
 {

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<winrt::hstring, winrt::Microsoft::Terminal::Settings::Model::Theme> GlobalAppSettings::Themes() noexcept

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<void()> handler)
+        {
+            _handler = std::move(handler);
+        }
+
+    private:
+        std::function<void()> _handler;
+    };
+
     template<typename T>
     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<std::function<void()>>;
+        // 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<SettingsWriteNotifier>;
         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();
             }
         }