Display a warning if SUI is unable to write to the settings file (#19027)

carlos-zamora · DHowett · commit 38f28e80d08d · 2025-06-24T16:48:10.000-05:00
Adds logic to display a warning popup if the settings.json is marked as read-only and we try to write to the settings.json file. Previously, this scenario would crash, which definitely isn't right. However, a simple fix of "not-crashing" wouldn't feel right either. This leverages the existing infrastructure to display a warning dialog when we failed to write to the settings file. The main annoyance here is that that popup dialog is located in `TerminalWindow` and is normally triggered from a failed `SettingsLoadEventArgs`. To get around this, `CascadiaSettings::WriteSettingsToDisk()` now returns a boolean to signal if the write was successful; whereas if it fails, a warning is added to the settings object. If we fail to write to disk, the function will return false and we'll raise an event with the settings' warnings to `TerminalPage` which passes it along to `TerminalWindow`. Additionally, this uses `IVectorView<SettingsLoadWarnings>` as opposed to `IVector<SettingsLoadWarnings>` throughout the relevant code. It's more correct as the list of warnings shouldn't be mutable and the warnings from the `CascadiaSettings` object are retrieved in that format. - ✅ Using SUI, save settings when the settings.json is set to read-only Closes #18913 (cherry picked from commit 218c9fb) Service-Card-Id: PVTI_lADOAF3p4s4Axadtzgb3QSk Service-Version: 1.23
diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp
@@ -392,7 +392,7 @@ namespace winrt::TerminalApp::implementation
                 auto ev = winrt::make_self<SettingsLoadEventArgs>(true,
                                                                   static_cast<uint64_t>(_settingsLoadedResult),
                                                                   _settingsLoadExceptionText,
-                                                                  warnings,
+                                                                  warnings.GetView(),
                                                                   _settings);
                 SettingsChanged.raise(*this, *ev);
                 return;
@@ -424,7 +424,7 @@ namespace winrt::TerminalApp::implementation
         auto ev = winrt::make_self<SettingsLoadEventArgs>(!initialLoad,
                                                           _settingsLoadedResult,
                                                           _settingsLoadExceptionText,
-                                                          warnings,
+                                                          warnings.GetView(),
                                                           _settings);
         SettingsChanged.raise(*this, *ev);
     }
@@ -491,7 +491,7 @@ namespace winrt::TerminalApp::implementation
         auto ev = winrt::make_self<SettingsLoadEventArgs>(false,
                                                           _settingsLoadedResult,
                                                           _settingsLoadExceptionText,
-                                                          warnings,
+                                                          warnings.GetView(),
                                                           _settings);
 
         auto window = winrt::make_self<implementation::TerminalWindow>(*ev, _contentManager);
diff --git a/src/cascadia/TerminalApp/SettingsLoadEventArgs.h b/src/cascadia/TerminalApp/SettingsLoadEventArgs.h
@@ -12,14 +12,14 @@ namespace winrt::TerminalApp::implementation
         WINRT_PROPERTY(bool, Reload, false);
         WINRT_PROPERTY(uint64_t, Result, S_OK);
         WINRT_PROPERTY(winrt::hstring, ExceptionText, L"");
-        WINRT_PROPERTY(winrt::Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>, Warnings, nullptr);
+        WINRT_PROPERTY(winrt::Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>, Warnings, nullptr);
         WINRT_PROPERTY(Microsoft::Terminal::Settings::Model::CascadiaSettings, NewSettings, nullptr);
 
     public:
         SettingsLoadEventArgs(bool reload,
                               uint64_t result,
                               winrt::hstring exceptionText,
-                              winrt::Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> warnings,
+                              winrt::Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> warnings,
                               Microsoft::Terminal::Settings::Model::CascadiaSettings newSettings) :
             _Reload{ reload },
             _Result{ result },
diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -4165,6 +4165,13 @@ namespace winrt::TerminalApp::implementation
             }
         });
 
+        sui.ShowLoadWarningsDialog([weakThis{ get_weak() }](auto&& /*s*/, const Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings) {
+            if (auto page{ weakThis.get() })
+            {
+                page->ShowLoadWarningsDialog.raise(*page, warnings);
+            }
+        });
+
         return *settingsContent;
     }
 
diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h
@@ -187,6 +187,7 @@ namespace winrt::TerminalApp::implementation
         til::typed_event<IInspectable, IInspectable> OpenSystemMenu;
         til::typed_event<IInspectable, IInspectable> QuitRequested;
         til::typed_event<IInspectable, winrt::Microsoft::Terminal::Control::ShowWindowArgs> ShowWindowChanged;
+        til::typed_event<Windows::Foundation::IInspectable, Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>> ShowLoadWarningsDialog;
 
         til::typed_event<Windows::Foundation::IInspectable, winrt::TerminalApp::RequestMoveContentArgs> RequestMoveContent;
         til::typed_event<Windows::Foundation::IInspectable, winrt::TerminalApp::RequestReceiveContentArgs> RequestReceiveContent;
diff --git a/src/cascadia/TerminalApp/TerminalPage.idl b/src/cascadia/TerminalApp/TerminalPage.idl
@@ -97,6 +97,7 @@ namespace TerminalApp
 
         event Windows.Foundation.TypedEventHandler<Object, Object> OpenSystemMenu;
         event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Control.ShowWindowArgs> ShowWindowChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Windows.Foundation.Collections.IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> > ShowLoadWarningsDialog;
 
         event Windows.Foundation.TypedEventHandler<Object, RequestMoveContentArgs> RequestMoveContent;
         event Windows.Foundation.TypedEventHandler<Object, RequestReceiveContentArgs> RequestReceiveContent;
diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp
@@ -219,6 +219,7 @@ namespace winrt::TerminalApp::implementation
         _root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized });
         _root->WindowSizeChanged({ get_weak(), &TerminalWindow::_WindowSizeChanged });
         _root->RenameWindowRequested({ get_weak(), &TerminalWindow::_RenameWindowRequested });
+        _root->ShowLoadWarningsDialog({ get_weak(), &TerminalWindow::_ShowLoadWarningsDialog });
         _root->Create();
 
         AppLogic::Current()->SettingsChanged({ get_weak(), &TerminalWindow::UpdateSettingsHandler });
@@ -476,7 +477,7 @@ namespace winrt::TerminalApp::implementation
     //   validating the settings.
     // - Only one dialog can be visible at a time. If another dialog is visible
     //   when this is called, nothing happens. See ShowDialog for details
-    void TerminalWindow::_ShowLoadWarningsDialog(const Windows::Foundation::Collections::IVector<SettingsLoadWarnings>& warnings)
+    void TerminalWindow::_ShowLoadWarningsDialog(const IInspectable&, const Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings>& warnings)
     {
         auto title = RS_(L"SettingsValidateErrorTitle");
         auto buttonText = RS_(L"Ok");
@@ -536,7 +537,7 @@ namespace winrt::TerminalApp::implementation
         }
         else if (settingsLoadedResult == S_FALSE)
         {
-            _ShowLoadWarningsDialog(_initialLoadResult.Warnings());
+            _ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());
         }
     }
 
@@ -822,7 +823,7 @@ namespace winrt::TerminalApp::implementation
         }
         else if (args.Result() == S_FALSE)
         {
-            _ShowLoadWarningsDialog(args.Warnings());
+            _ShowLoadWarningsDialog(nullptr, args.Warnings());
         }
         else if (args.Result() == S_OK)
         {
diff --git a/src/cascadia/TerminalApp/TerminalWindow.h b/src/cascadia/TerminalApp/TerminalWindow.h
@@ -189,7 +189,7 @@ namespace winrt::TerminalApp::implementation
                                    const winrt::hstring& contentKey,
                                    HRESULT settingsLoadedResult,
                                    const winrt::hstring& exceptionText);
-        void _ShowLoadWarningsDialog(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings);
+        void _ShowLoadWarningsDialog(const IInspectable& sender, const Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings);
 
         bool _IsKeyboardServiceEnabled();
 
diff --git a/src/cascadia/TerminalApp/TerminalWindow.idl b/src/cascadia/TerminalApp/TerminalWindow.idl
@@ -30,7 +30,7 @@ namespace TerminalApp
     {
         Boolean Reload { get; };
         UInt64 Result { get; };
-        IVector<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };
+        IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };
         String ExceptionText { get; };
 
         Microsoft.Terminal.Settings.Model.CascadiaSettings NewSettings { get; };
diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp
@@ -590,7 +590,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
     void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)
     {
         _settingsClone.LogSettingChanges(false);
-        _settingsClone.WriteSettingsToDisk();
+        if (!_settingsClone.WriteSettingsToDisk())
+        {
+            ShowLoadWarningsDialog.raise(*this, _settingsClone.Warnings());
+        }
     }
 
     void MainPage::ResetButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)
diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.h b/src/cascadia/TerminalSettingsEditor/MainPage.h
@@ -45,6 +45,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         Windows::Foundation::Collections::IObservableVector<IInspectable> Breadcrumbs() noexcept;
 
         til::typed_event<Windows::Foundation::IInspectable, Model::SettingsTarget> OpenJson;
+        til::typed_event<Windows::Foundation::IInspectable, Windows::Foundation::Collections::IVectorView<Model::SettingsLoadWarnings>> ShowLoadWarningsDialog;
 
     private:
         Windows::Foundation::Collections::IObservableVector<IInspectable> _breadcrumbs;
diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.idl b/src/cascadia/TerminalSettingsEditor/MainPage.idl
@@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Editor
 
         void UpdateSettings(Microsoft.Terminal.Settings.Model.CascadiaSettings settings);
         event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Settings.Model.SettingsTarget> OpenJson;
+        event Windows.Foundation.TypedEventHandler<Object, Windows.Foundation.Collections.IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> > ShowLoadWarningsDialog;
 
         // Due to the aforementioned bug, we can't use IInitializeWithWindow _here_ either.
         // Let's just smuggle the HWND in as a UInt64 :|
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
@@ -127,7 +127,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> AllProfiles() const noexcept;
         winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> ActiveProfiles() const noexcept;
         Model::ActionMap ActionMap() const noexcept;
-        void WriteSettingsToDisk();
+        bool WriteSettingsToDisk();
         Json::Value ToJson() const;
         Model::Profile ProfileDefaults() const;
         Model::Profile CreateNewProfile();
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl
@@ -23,7 +23,7 @@ namespace Microsoft.Terminal.Settings.Model
         CascadiaSettings(String userJSON, String inboxJSON);
 
         CascadiaSettings Copy();
-        void WriteSettingsToDisk();
+        Boolean WriteSettingsToDisk();
         void LogSettingChanges(Boolean isJsonLoad);
 
         String Hash { get; };
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -1056,15 +1056,7 @@ try
     // settings string back to the file.
     if (mustWriteToDisk)
     {
-        try
-        {
-            settings->WriteSettingsToDisk();
-        }
-        catch (...)
-        {
-            LOG_CAUGHT_EXCEPTION();
-            settings->_warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings);
-        }
+        settings->WriteSettingsToDisk();
     }
     else
     {
@@ -1354,7 +1346,7 @@ winrt::hstring CascadiaSettings::DefaultSettingsPath()
 // - <none>
 // Return Value:
 // - <none>
-void CascadiaSettings::WriteSettingsToDisk()
+bool CascadiaSettings::WriteSettingsToDisk()
 {
     const auto settingsPath = _settingsPath();
 
@@ -1364,18 +1356,28 @@ void CascadiaSettings::WriteSettingsToDisk()
     wbuilder.settings_["indentation"] = "    ";
     wbuilder.settings_["precision"] = 6; // prevent values like 1.1000000000000001
 
-    FILETIME lastWriteTime{};
-    const auto styledString{ Json::writeString(wbuilder, ToJson()) };
-    til::io::write_utf8_string_to_file_atomic(settingsPath, styledString, &lastWriteTime);
+    try
+    {
+        FILETIME lastWriteTime{};
+        const auto styledString{ Json::writeString(wbuilder, ToJson()) };
+        til::io::write_utf8_string_to_file_atomic(settingsPath, styledString, &lastWriteTime);
 
-    _hash = _calculateHash(styledString, lastWriteTime);
+        _hash = _calculateHash(styledString, lastWriteTime);
 
-    // Persists the default terminal choice
-    // GH#10003 - Only do this if _currentDefaultTerminal was actually initialized.
-    if (_currentDefaultTerminal)
+        // Persists the default terminal choice
+        // GH#10003 - Only do this if _currentDefaultTerminal was actually initialized.
+        if (_currentDefaultTerminal)
+        {
+            DefaultTerminal::Current(_currentDefaultTerminal);
+        }
+    }
+    catch (...)
     {
-        DefaultTerminal::Current(_currentDefaultTerminal);
+        LOG_CAUGHT_EXCEPTION();
+        _warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings);
+        return false;
     }
+    return true;
 }
 
 #ifndef NDEBUG
diff --git a/src/inc/til/io.h b/src/inc/til/io.h
@@ -256,7 +256,11 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
             // renaming one is (supposed to be) atomic.
             // Wait... "supposed to be"!? Well it's technically not always atomic,
             // but it's pretty darn close to it, so... better than nothing.
-            std::filesystem::rename(tmpPath, resolvedPath);
+            std::filesystem::rename(tmpPath, resolvedPath, ec);
+            if (ec)
+            {
+                THROW_WIN32_MSG(ec.value(), "failed to write to file");
+            }
         }
     } // io
 } // til

Original file line number	Diff line number	Diff line change
`@@ -4165,6 +4165,13 @@ namespace winrt::TerminalApp::implementation`
`4165`	`4165`	`}`
`4166`	`4166`	`});`
`4167`	`4167`
	`4168`	`+ sui.ShowLoadWarningsDialog([weakThis{ get_weak() }](auto&& /s/, const Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings) {`
	`4169`	`+ if (auto page{ weakThis.get() })`
	`4170`	`+ {`
	`4171`	`+ page->ShowLoadWarningsDialog.raise(*page, warnings);`
	`4172`	`+ }`
	`4173`	`+ });`
	`4174`	`+`
`4168`	`4175`	`return *settingsContent;`
`4169`	`4176`	`}`
`4170`	`4177`
Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,7 @@ namespace winrt::TerminalApp::implementation`
`219`	`219`	`_root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized });`
`220`	`220`	`_root->WindowSizeChanged({ get_weak(), &TerminalWindow::_WindowSizeChanged });`
`221`	`221`	`_root->RenameWindowRequested({ get_weak(), &TerminalWindow::_RenameWindowRequested });`
	`222`	`+ _root->ShowLoadWarningsDialog({ get_weak(), &TerminalWindow::_ShowLoadWarningsDialog });`
`222`	`223`	`_root->Create();`
`223`	`224`
`224`	`225`	`AppLogic::Current()->SettingsChanged({ get_weak(), &TerminalWindow::UpdateSettingsHandler });`
`@@ -476,7 +477,7 @@ namespace winrt::TerminalApp::implementation`
`476`	`477`	`// validating the settings.`
`477`	`478`	`// - Only one dialog can be visible at a time. If another dialog is visible`
`478`	`479`	`// when this is called, nothing happens. See ShowDialog for details`
`479`		`- void TerminalWindow::_ShowLoadWarningsDialog(const Windows::Foundation::Collections::IVector<SettingsLoadWarnings>& warnings)`
	`480`	`+ void TerminalWindow::_ShowLoadWarningsDialog(const IInspectable&, const Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings>& warnings)`
`480`	`481`	`{`
`481`	`482`	`auto title = RS_(L"SettingsValidateErrorTitle");`
`482`	`483`	`auto buttonText = RS_(L"Ok");`
`@@ -536,7 +537,7 @@ namespace winrt::TerminalApp::implementation`
`536`	`537`	`}`
`537`	`538`	`else if (settingsLoadedResult == S_FALSE)`
`538`	`539`	`{`
`539`		`- _ShowLoadWarningsDialog(_initialLoadResult.Warnings());`
	`540`	`+ _ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());`
`540`	`541`	`}`
`541`	`542`	`}`
`542`	`543`
`@@ -822,7 +823,7 @@ namespace winrt::TerminalApp::implementation`
`822`	`823`	`}`
`823`	`824`	`else if (args.Result() == S_FALSE)`
`824`	`825`	`{`
`825`		`- _ShowLoadWarningsDialog(args.Warnings());`
	`826`	`+ _ShowLoadWarningsDialog(nullptr, args.Warnings());`
`826`	`827`	`}`
`827`	`828`	`else if (args.Result() == S_OK)`
`828`	`829`	`{`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ namespace TerminalApp`
`30`	`30`	`{`
`31`	`31`	`Boolean Reload { get; };`
`32`	`32`	`UInt64 Result { get; };`
`33`		`- IVector<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };`
	`33`	`+ IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };`
`34`	`34`	`String ExceptionText { get; };`
`35`	`35`
`36`	`36`	`Microsoft.Terminal.Settings.Model.CascadiaSettings NewSettings { get; };`
Original file line number	Diff line number	Diff line change
`@@ -590,7 +590,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation`
`590`	`590`	`void MainPage::SaveButton_Click(const IInspectable& /sender/, const RoutedEventArgs& /args/)`
`591`	`591`	`{`
`592`	`592`	`_settingsClone.LogSettingChanges(false);`
`593`		`- _settingsClone.WriteSettingsToDisk();`
	`593`	`+ if (!_settingsClone.WriteSettingsToDisk())`
	`594`	`+ {`
	`595`	`+ ShowLoadWarningsDialog.raise(*this, _settingsClone.Warnings());`
	`596`	`+ }`
`594`	`597`	`}`
`595`	`598`
`596`	`599`	`void MainPage::ResetButton_Click(const IInspectable& /sender/, const RoutedEventArgs& /args/)`