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

carlos-zamora · DHowett · commit 92daa011bdb0 · 2025-06-24T16:46:41.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_lADOAF3p4s4AmhmQzgb3QSg Service-Version: 1.22
diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp
@@ -396,7 +396,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;
@@ -428,7 +428,7 @@ namespace winrt::TerminalApp::implementation
         auto ev = winrt::make_self<SettingsLoadEventArgs>(!initialLoad,
                                                           _settingsLoadedResult,
                                                           _settingsLoadExceptionText,
-                                                          warnings,
+                                                          warnings.GetView(),
                                                           _settings);
         SettingsChanged.raise(*this, *ev);
     }
@@ -656,7 +656,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
@@ -4097,6 +4097,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
@@ -199,6 +199,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
@@ -103,6 +103,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
@@ -217,6 +217,7 @@ namespace winrt::TerminalApp::implementation
         _root->SetSettings(_settings, false); // We're on our UI thread right now, so this is safe
         _root->Loaded({ get_weak(), &TerminalWindow::_OnLoaded });
         _root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized });
+        _root->ShowLoadWarningsDialog({ get_weak(), &TerminalWindow::_ShowLoadWarningsDialog });
         _root->Create();
 
         AppLogic::Current()->SettingsChanged({ get_weak(), &TerminalWindow::UpdateSettingsHandler });
@@ -441,7 +442,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");
@@ -501,7 +502,7 @@ namespace winrt::TerminalApp::implementation
         }
         else if (settingsLoadedResult == S_FALSE)
         {
-            _ShowLoadWarningsDialog(_initialLoadResult.Warnings());
+            _ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());
         }
     }
 
@@ -802,7 +803,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
@@ -194,7 +194,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
@@ -496,7 +496,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
@@ -35,6 +35,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
`@@ -4097,6 +4097,13 @@ namespace winrt::TerminalApp::implementation`
`4097`	`4097`	`}`
`4098`	`4098`	`});`
`4099`	`4099`
	`4100`	`+ sui.ShowLoadWarningsDialog([weakThis{ get_weak() }](auto&& /s/, const Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings) {`
	`4101`	`+ if (auto page{ weakThis.get() })`
	`4102`	`+ {`
	`4103`	`+ page->ShowLoadWarningsDialog.raise(*page, warnings);`
	`4104`	`+ }`
	`4105`	`+ });`
	`4106`	`+`
`4100`	`4107`	`return *settingsContent;`
`4101`	`4108`	`}`
`4102`	`4109`
Original file line number	Diff line number	Diff line change
`@@ -217,6 +217,7 @@ namespace winrt::TerminalApp::implementation`
`217`	`217`	`_root->SetSettings(_settings, false); // We're on our UI thread right now, so this is safe`
`218`	`218`	`_root->Loaded({ get_weak(), &TerminalWindow::_OnLoaded });`
`219`	`219`	`_root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized });`
	`220`	`+ _root->ShowLoadWarningsDialog({ get_weak(), &TerminalWindow::_ShowLoadWarningsDialog });`
`220`	`221`	`_root->Create();`
`221`	`222`
`222`	`223`	`AppLogic::Current()->SettingsChanged({ get_weak(), &TerminalWindow::UpdateSettingsHandler });`
`@@ -441,7 +442,7 @@ namespace winrt::TerminalApp::implementation`
`441`	`442`	`// validating the settings.`
`442`	`443`	`// - Only one dialog can be visible at a time. If another dialog is visible`
`443`	`444`	`// when this is called, nothing happens. See ShowDialog for details`
`444`		`- void TerminalWindow::_ShowLoadWarningsDialog(const Windows::Foundation::Collections::IVector<SettingsLoadWarnings>& warnings)`
	`445`	`+ void TerminalWindow::_ShowLoadWarningsDialog(const IInspectable&, const Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings>& warnings)`
`445`	`446`	`{`
`446`	`447`	`auto title = RS_(L"SettingsValidateErrorTitle");`
`447`	`448`	`auto buttonText = RS_(L"Ok");`
`@@ -501,7 +502,7 @@ namespace winrt::TerminalApp::implementation`
`501`	`502`	`}`
`502`	`503`	`else if (settingsLoadedResult == S_FALSE)`
`503`	`504`	`{`
`504`		`- _ShowLoadWarningsDialog(_initialLoadResult.Warnings());`
	`505`	`+ _ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());`
`505`	`506`	`}`
`506`	`507`	`}`
`507`	`508`
`@@ -802,7 +803,7 @@ namespace winrt::TerminalApp::implementation`
`802`	`803`	`}`
`803`	`804`	`else if (args.Result() == S_FALSE)`
`804`	`805`	`{`
`805`		`- _ShowLoadWarningsDialog(args.Warnings());`
	`806`	`+ _ShowLoadWarningsDialog(nullptr, args.Warnings());`
`806`	`807`	`}`
`807`	`808`	`else if (args.Result() == S_OK)`
`808`	`809`	`{`
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
`@@ -496,7 +496,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation`
`496`	`496`	`void MainPage::SaveButton_Click(const IInspectable& /sender/, const RoutedEventArgs& /args/)`
`497`	`497`	`{`
`498`	`498`	`_settingsClone.LogSettingChanges(false);`
`499`		`- _settingsClone.WriteSettingsToDisk();`
	`499`	`+ if (!_settingsClone.WriteSettingsToDisk())`
	`500`	`+ {`
	`501`	`+ ShowLoadWarningsDialog.raise(*this, _settingsClone.Warnings());`
	`502`	`+ }`
`500`	`503`	`}`
`501`	`504`
`502`	`505`	`void MainPage::ResetButton_Click(const IInspectable& /sender/, const RoutedEventArgs& /args/)`