Display a warning if SUI is unable to write to settings file #19027
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Demo
Urgh. I guess the dialog's title is slightly incorrect. Can be fixed if desired. |
carlos-zamora
commented
Jun 13, 2025
| @@ -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; | |||
There was a problem hiding this comment.
Fun fact: you need the space here - SettingsLoadWarnings> > ShowLoadWarningsDialog or else the MIDL compiler gets angry and confused
PankajBhojwani
approved these changes
Jun 23, 2025
DHowett
reviewed
Jun 24, 2025
| @@ -220,6 +220,12 @@ namespace winrt::TerminalApp::implementation | |||
| _root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized }); | |||
| _root->WindowSizeChanged({ get_weak(), &TerminalWindow::_WindowSizeChanged }); | |||
| _root->RenameWindowRequested({ get_weak(), &TerminalWindow::_RenameWindowRequested }); | |||
| _root->ShowLoadWarningsDialog([weakThis{ get_weak() }](auto&& /*s*/, const Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings) { | |||
There was a problem hiding this comment.
in general, this should follow the same pattern as above. Use {get_weak(), &Foo::Function} instead of authoring your own weak reference resolver. :)
It will require adding a useless argument to _ShowLoadWarningsDialog, but perhaps that's fine
PankajBhojwani
approved these changes
Jun 24, 2025
DHowett
pushed a commit
that referenced
this pull request
Jun 24, 2025
) 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
DHowett
pushed a commit
that referenced
this pull request
Jun 24, 2025
) 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
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
TerminalWindowand is normally triggered from a failedSettingsLoadEventArgs. 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 toTerminalPagewhich passes it along toTerminalWindow.Additionally, this uses
IVectorView<SettingsLoadWarnings>as opposed toIVector<SettingsLoadWarnings>throughout the relevant code. It's more correct as the list of warnings shouldn't be mutable and the warnings from theCascadiaSettingsobject are retrieved in that format.Validation Steps Performed
Closes #18913