Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea6a0d6
Display a warning if SUI is unable to write to settings file
carlos-zamora Jun 13, 2025
6468de2
Dispense with the lambda on _ShowLoadWarningsDialog
DHowett Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/SettingsLoadEventArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down
7 changes: 7 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TerminalPage.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact: you need the space here - SettingsLoadWarnings> > ShowLoadWarningsDialog or else the MIDL compiler gets angry and confused

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! C++03 template rules!


event Windows.Foundation.TypedEventHandler<Object, RequestMoveContentArgs> RequestMoveContent;
event Windows.Foundation.TypedEventHandler<Object, RequestReceiveContentArgs> RequestReceiveContent;
Expand Down
7 changes: 4 additions & 3 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down Expand Up @@ -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<SettingsLoadWarnings>& warnings)
void TerminalWindow::_ShowLoadWarningsDialog(const IInspectable&, const Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings>& warnings)
{
auto title = RS_(L"SettingsValidateErrorTitle");
auto buttonText = RS_(L"Ok");
Expand Down Expand Up @@ -539,7 +540,7 @@ namespace winrt::TerminalApp::implementation
}
else if (settingsLoadedResult == S_FALSE)
{
_ShowLoadWarningsDialog(_initialLoadResult.Warnings());
_ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());
}
}

Expand Down Expand Up @@ -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)
{
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalWindow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalWindow.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; };
Expand Down
5 changes: 4 additions & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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*/)
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/MainPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Editor::ExtensionsViewModel ExtensionsVM() const noexcept { return _extensionsVM; }

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;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/MainPage.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,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 :|
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
winrt::Windows::Foundation::Collections::IVectorView<Model::ExtensionPackage> Extensions();
void ResetApplicationState() const;
void ResetToDefaultSettings();
void WriteSettingsToDisk();
bool WriteSettingsToDisk();
Json::Value ToJson() const;
Model::Profile ProfileDefaults() const;
Model::Profile CreateNewProfile();
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettings.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; };
Expand Down
24 changes: 13 additions & 11 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -1539,15 +1531,25 @@ void CascadiaSettings::ResetToDefaultSettings()
// - <none>
// Return Value:
// - <none>
void CascadiaSettings::WriteSettingsToDisk()
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

_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)
Expand Down
6 changes: 5 additions & 1 deletion src/inc/til/io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading