From 218c9fbe3ecd79cc15d9cab267753b1b68ead250 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 24 Jun 2025 13:06:03 -0700 Subject: [PATCH] Display a warning if SUI is unable to write to the settings file (#19027) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` as opposed to `IVector` 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. ## Validation Steps Performed - ✅ Using SUI, save settings when the settings.json is set to read-only Closes #18913 --- src/cascadia/TerminalApp/AppLogic.cpp | 6 ++--- .../TerminalApp/SettingsLoadEventArgs.h | 4 ++-- src/cascadia/TerminalApp/TerminalPage.cpp | 7 ++++++ src/cascadia/TerminalApp/TerminalPage.h | 1 + src/cascadia/TerminalApp/TerminalPage.idl | 1 + src/cascadia/TerminalApp/TerminalWindow.cpp | 7 +++--- src/cascadia/TerminalApp/TerminalWindow.h | 2 +- src/cascadia/TerminalApp/TerminalWindow.idl | 2 +- .../TerminalSettingsEditor/MainPage.cpp | 5 +++- .../TerminalSettingsEditor/MainPage.h | 1 + .../TerminalSettingsEditor/MainPage.idl | 1 + .../TerminalSettingsModel/CascadiaSettings.h | 2 +- .../CascadiaSettings.idl | 2 +- .../CascadiaSettingsSerialization.cpp | 24 ++++++++++--------- src/inc/til/io.h | 6 ++++- 15 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index b7e0eae91a..6a0d456142 100644 --- 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(true, static_cast(_settingsLoadedResult), _settingsLoadExceptionText, - warnings, + warnings.GetView(), _settings); SettingsChanged.raise(*this, *ev); return; @@ -424,7 +424,7 @@ namespace winrt::TerminalApp::implementation auto ev = winrt::make_self(!initialLoad, _settingsLoadedResult, _settingsLoadExceptionText, - warnings, + warnings.GetView(), _settings); SettingsChanged.raise(*this, *ev); } @@ -491,7 +491,7 @@ namespace winrt::TerminalApp::implementation auto ev = winrt::make_self(false, _settingsLoadedResult, _settingsLoadExceptionText, - warnings, + warnings.GetView(), _settings); auto window = winrt::make_self(*ev, _contentManager); diff --git a/src/cascadia/TerminalApp/SettingsLoadEventArgs.h b/src/cascadia/TerminalApp/SettingsLoadEventArgs.h index e094810784..f9436f95ae 100644 --- 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, Warnings, nullptr); + WINRT_PROPERTY(winrt::Windows::Foundation::Collections::IVectorView, 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 warnings, + winrt::Windows::Foundation::Collections::IVectorView 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 index 702b4a36ee..810ce5272b 100644 --- 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& 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 index b369bd920e..1fd773fb3b 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -187,6 +187,7 @@ namespace winrt::TerminalApp::implementation til::typed_event OpenSystemMenu; til::typed_event QuitRequested; til::typed_event ShowWindowChanged; + til::typed_event> ShowLoadWarningsDialog; til::typed_event RequestMoveContent; til::typed_event RequestReceiveContent; diff --git a/src/cascadia/TerminalApp/TerminalPage.idl b/src/cascadia/TerminalApp/TerminalPage.idl index 2788cedd10..f1bd4fbc66 100644 --- a/src/cascadia/TerminalApp/TerminalPage.idl +++ b/src/cascadia/TerminalApp/TerminalPage.idl @@ -97,6 +97,7 @@ namespace TerminalApp event Windows.Foundation.TypedEventHandler OpenSystemMenu; event Windows.Foundation.TypedEventHandler ShowWindowChanged; + event Windows.Foundation.TypedEventHandler > ShowLoadWarningsDialog; event Windows.Foundation.TypedEventHandler RequestMoveContent; event Windows.Foundation.TypedEventHandler RequestReceiveContent; diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 65518b5fdc..88a839949c 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -220,6 +220,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 }); @@ -478,7 +479,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& warnings) + void TerminalWindow::_ShowLoadWarningsDialog(const IInspectable&, const Windows::Foundation::Collections::IVectorView& warnings) { auto title = RS_(L"SettingsValidateErrorTitle"); auto buttonText = RS_(L"Ok"); @@ -539,7 +540,7 @@ namespace winrt::TerminalApp::implementation } else if (settingsLoadedResult == S_FALSE) { - _ShowLoadWarningsDialog(_initialLoadResult.Warnings()); + _ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings()); } } @@ -825,7 +826,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 index fd27617846..a7db7a8034 100644 --- 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& warnings); + void _ShowLoadWarningsDialog(const IInspectable& sender, const Windows::Foundation::Collections::IVectorView& warnings); bool _IsKeyboardServiceEnabled(); diff --git a/src/cascadia/TerminalApp/TerminalWindow.idl b/src/cascadia/TerminalApp/TerminalWindow.idl index baf58317df..35ccfe9340 100644 --- 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 Warnings { get; }; + IVectorView 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 index 098827d9a9..b5fdc867b6 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -680,7 +680,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 index aebeb516e3..ff87f4910b 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.h +++ b/src/cascadia/TerminalSettingsEditor/MainPage.h @@ -46,6 +46,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation Editor::ExtensionsViewModel ExtensionsVM() const noexcept { return _extensionsVM; } til::typed_event OpenJson; + til::typed_event> ShowLoadWarningsDialog; private: Windows::Foundation::Collections::IObservableVector _breadcrumbs; diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.idl b/src/cascadia/TerminalSettingsEditor/MainPage.idl index 390ae5cb80..20f3bab869 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.idl +++ b/src/cascadia/TerminalSettingsEditor/MainPage.idl @@ -39,6 +39,7 @@ namespace Microsoft.Terminal.Settings.Editor void UpdateSettings(Microsoft.Terminal.Settings.Model.CascadiaSettings settings); event Windows.Foundation.TypedEventHandler OpenJson; + event Windows.Foundation.TypedEventHandler > 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 index a4fd16bd43..defd04af69 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -167,7 +167,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::Windows::Foundation::Collections::IVectorView Extensions(); void ResetApplicationState() const; void ResetToDefaultSettings(); - 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 index 95165bcc80..d1f4304ba9 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -31,7 +31,7 @@ namespace Microsoft.Terminal.Settings.Model CascadiaSettings Copy(); void ResetApplicationState(); void ResetToDefaultSettings(); - 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 index 6e03785dec..bbbb9172c7 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -1226,15 +1226,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 { @@ -1539,7 +1531,7 @@ void CascadiaSettings::ResetToDefaultSettings() // - // Return Value: // - -void CascadiaSettings::WriteSettingsToDisk() +bool CascadiaSettings::WriteSettingsToDisk() { // write current settings to current settings file Json::StreamWriterBuilder wbuilder; @@ -1547,7 +1539,17 @@ void CascadiaSettings::WriteSettingsToDisk() wbuilder.settings_["indentation"] = " "; wbuilder.settings_["precision"] = 6; // prevent values like 1.1000000000000001 - _writeSettingsToDisk(Json::writeString(wbuilder, ToJson())); + 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) diff --git a/src/inc/til/io.h b/src/inc/til/io.h index 6f917f0d0b..9e28b99630 100644 --- 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