Skip to content

Commit 38f28e8

Browse files
carlos-zamoraDHowett
carlos-zamora
authored and
DHowett
committed
Display a warning if SUI is unable to write to the settings file (#19027)
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
1 parent 820b10f commit 38f28e8

File tree

15 files changed

+53
-32
lines changed

15 files changed

+53
-32
lines changed

‎src/cascadia/TerminalApp/AppLogic.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ namespace winrt::TerminalApp::implementation
392392
auto ev = winrt::make_self<SettingsLoadEventArgs>(true,
393393
static_cast<uint64_t>(_settingsLoadedResult),
394394
_settingsLoadExceptionText,
395-
warnings,
395+
warnings.GetView(),
396396
_settings);
397397
SettingsChanged.raise(*this, *ev);
398398
return;
@@ -424,7 +424,7 @@ namespace winrt::TerminalApp::implementation
424424
auto ev = winrt::make_self<SettingsLoadEventArgs>(!initialLoad,
425425
_settingsLoadedResult,
426426
_settingsLoadExceptionText,
427-
warnings,
427+
warnings.GetView(),
428428
_settings);
429429
SettingsChanged.raise(*this, *ev);
430430
}
@@ -491,7 +491,7 @@ namespace winrt::TerminalApp::implementation
491491
auto ev = winrt::make_self<SettingsLoadEventArgs>(false,
492492
_settingsLoadedResult,
493493
_settingsLoadExceptionText,
494-
warnings,
494+
warnings.GetView(),
495495
_settings);
496496

497497
auto window = winrt::make_self<implementation::TerminalWindow>(*ev, _contentManager);

‎src/cascadia/TerminalApp/SettingsLoadEventArgs.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,14 @@ namespace winrt::TerminalApp::implementation
1212
WINRT_PROPERTY(bool, Reload, false);
1313
WINRT_PROPERTY(uint64_t, Result, S_OK);
1414
WINRT_PROPERTY(winrt::hstring, ExceptionText, L"");
15-
WINRT_PROPERTY(winrt::Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>, Warnings, nullptr);
15+
WINRT_PROPERTY(winrt::Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>, Warnings, nullptr);
1616
WINRT_PROPERTY(Microsoft::Terminal::Settings::Model::CascadiaSettings, NewSettings, nullptr);
1717

1818
public:
1919
SettingsLoadEventArgs(bool reload,
2020
uint64_t result,
2121
winrt::hstring exceptionText,
22-
winrt::Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> warnings,
22+
winrt::Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> warnings,
2323
Microsoft::Terminal::Settings::Model::CascadiaSettings newSettings) :
2424
_Reload{ reload },
2525
_Result{ result },

‎src/cascadia/TerminalApp/TerminalPage.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4165,6 +4165,13 @@ namespace winrt::TerminalApp::implementation
41654165
}
41664166
});
41674167

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+
41684175
return *settingsContent;
41694176
}
41704177

‎src/cascadia/TerminalApp/TerminalPage.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ namespace winrt::TerminalApp::implementation
187187
til::typed_event<IInspectable, IInspectable> OpenSystemMenu;
188188
til::typed_event<IInspectable, IInspectable> QuitRequested;
189189
til::typed_event<IInspectable, winrt::Microsoft::Terminal::Control::ShowWindowArgs> ShowWindowChanged;
190+
til::typed_event<Windows::Foundation::IInspectable, Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>> ShowLoadWarningsDialog;
190191

191192
til::typed_event<Windows::Foundation::IInspectable, winrt::TerminalApp::RequestMoveContentArgs> RequestMoveContent;
192193
til::typed_event<Windows::Foundation::IInspectable, winrt::TerminalApp::RequestReceiveContentArgs> RequestReceiveContent;

‎src/cascadia/TerminalApp/TerminalPage.idl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ namespace TerminalApp
9797

9898
event Windows.Foundation.TypedEventHandler<Object, Object> OpenSystemMenu;
9999
event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Control.ShowWindowArgs> ShowWindowChanged;
100+
event Windows.Foundation.TypedEventHandler<Object, Windows.Foundation.Collections.IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> > ShowLoadWarningsDialog;
100101

101102
event Windows.Foundation.TypedEventHandler<Object, RequestMoveContentArgs> RequestMoveContent;
102103
event Windows.Foundation.TypedEventHandler<Object, RequestReceiveContentArgs> RequestReceiveContent;

‎src/cascadia/TerminalApp/TerminalWindow.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ namespace winrt::TerminalApp::implementation
219219
_root->Initialized({ get_weak(), &TerminalWindow::_pageInitialized });
220220
_root->WindowSizeChanged({ get_weak(), &TerminalWindow::_WindowSizeChanged });
221221
_root->RenameWindowRequested({ get_weak(), &TerminalWindow::_RenameWindowRequested });
222+
_root->ShowLoadWarningsDialog({ get_weak(), &TerminalWindow::_ShowLoadWarningsDialog });
222223
_root->Create();
223224

224225
AppLogic::Current()->SettingsChanged({ get_weak(), &TerminalWindow::UpdateSettingsHandler });
@@ -476,7 +477,7 @@ namespace winrt::TerminalApp::implementation
476477
// validating the settings.
477478
// - Only one dialog can be visible at a time. If another dialog is visible
478479
// 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)
480481
{
481482
auto title = RS_(L"SettingsValidateErrorTitle");
482483
auto buttonText = RS_(L"Ok");
@@ -536,7 +537,7 @@ namespace winrt::TerminalApp::implementation
536537
}
537538
else if (settingsLoadedResult == S_FALSE)
538539
{
539-
_ShowLoadWarningsDialog(_initialLoadResult.Warnings());
540+
_ShowLoadWarningsDialog(nullptr, _initialLoadResult.Warnings());
540541
}
541542
}
542543

@@ -822,7 +823,7 @@ namespace winrt::TerminalApp::implementation
822823
}
823824
else if (args.Result() == S_FALSE)
824825
{
825-
_ShowLoadWarningsDialog(args.Warnings());
826+
_ShowLoadWarningsDialog(nullptr, args.Warnings());
826827
}
827828
else if (args.Result() == S_OK)
828829
{

‎src/cascadia/TerminalApp/TerminalWindow.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ namespace winrt::TerminalApp::implementation
189189
const winrt::hstring& contentKey,
190190
HRESULT settingsLoadedResult,
191191
const winrt::hstring& exceptionText);
192-
void _ShowLoadWarningsDialog(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings);
192+
void _ShowLoadWarningsDialog(const IInspectable& sender, const Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings>& warnings);
193193

194194
bool _IsKeyboardServiceEnabled();
195195

‎src/cascadia/TerminalApp/TerminalWindow.idl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ namespace TerminalApp
3030
{
3131
Boolean Reload { get; };
3232
UInt64 Result { get; };
33-
IVector<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };
33+
IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> Warnings { get; };
3434
String ExceptionText { get; };
3535

3636
Microsoft.Terminal.Settings.Model.CascadiaSettings NewSettings { get; };

‎src/cascadia/TerminalSettingsEditor/MainPage.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
590590
void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)
591591
{
592592
_settingsClone.LogSettingChanges(false);
593-
_settingsClone.WriteSettingsToDisk();
593+
if (!_settingsClone.WriteSettingsToDisk())
594+
{
595+
ShowLoadWarningsDialog.raise(*this, _settingsClone.Warnings());
596+
}
594597
}
595598

596599
void MainPage::ResetButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)

‎src/cascadia/TerminalSettingsEditor/MainPage.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
4545
Windows::Foundation::Collections::IObservableVector<IInspectable> Breadcrumbs() noexcept;
4646

4747
til::typed_event<Windows::Foundation::IInspectable, Model::SettingsTarget> OpenJson;
48+
til::typed_event<Windows::Foundation::IInspectable, Windows::Foundation::Collections::IVectorView<Model::SettingsLoadWarnings>> ShowLoadWarningsDialog;
4849

4950
private:
5051
Windows::Foundation::Collections::IObservableVector<IInspectable> _breadcrumbs;

‎src/cascadia/TerminalSettingsEditor/MainPage.idl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Editor
3636

3737
void UpdateSettings(Microsoft.Terminal.Settings.Model.CascadiaSettings settings);
3838
event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Settings.Model.SettingsTarget> OpenJson;
39+
event Windows.Foundation.TypedEventHandler<Object, Windows.Foundation.Collections.IVectorView<Microsoft.Terminal.Settings.Model.SettingsLoadWarnings> > ShowLoadWarningsDialog;
3940

4041
// Due to the aforementioned bug, we can't use IInitializeWithWindow _here_ either.
4142
// Let's just smuggle the HWND in as a UInt64 :|

‎src/cascadia/TerminalSettingsModel/CascadiaSettings.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
127127
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> AllProfiles() const noexcept;
128128
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> ActiveProfiles() const noexcept;
129129
Model::ActionMap ActionMap() const noexcept;
130-
void WriteSettingsToDisk();
130+
bool WriteSettingsToDisk();
131131
Json::Value ToJson() const;
132132
Model::Profile ProfileDefaults() const;
133133
Model::Profile CreateNewProfile();

‎src/cascadia/TerminalSettingsModel/CascadiaSettings.idl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ namespace Microsoft.Terminal.Settings.Model
2323
CascadiaSettings(String userJSON, String inboxJSON);
2424

2525
CascadiaSettings Copy();
26-
void WriteSettingsToDisk();
26+
Boolean WriteSettingsToDisk();
2727
void LogSettingChanges(Boolean isJsonLoad);
2828

2929
String Hash { get; };

‎src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,15 +1056,7 @@ try
10561056
// settings string back to the file.
10571057
if (mustWriteToDisk)
10581058
{
1059-
try
1060-
{
1061-
settings->WriteSettingsToDisk();
1062-
}
1063-
catch (...)
1064-
{
1065-
LOG_CAUGHT_EXCEPTION();
1066-
settings->_warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings);
1067-
}
1059+
settings->WriteSettingsToDisk();
10681060
}
10691061
else
10701062
{
@@ -1354,7 +1346,7 @@ winrt::hstring CascadiaSettings::DefaultSettingsPath()
13541346
// - <none>
13551347
// Return Value:
13561348
// - <none>
1357-
void CascadiaSettings::WriteSettingsToDisk()
1349+
bool CascadiaSettings::WriteSettingsToDisk()
13581350
{
13591351
const auto settingsPath = _settingsPath();
13601352

@@ -1364,18 +1356,28 @@ void CascadiaSettings::WriteSettingsToDisk()
13641356
wbuilder.settings_["indentation"] = " ";
13651357
wbuilder.settings_["precision"] = 6; // prevent values like 1.1000000000000001
13661358

1367-
FILETIME lastWriteTime{};
1368-
const auto styledString{ Json::writeString(wbuilder, ToJson()) };
1369-
til::io::write_utf8_string_to_file_atomic(settingsPath, styledString, &lastWriteTime);
1359+
try
1360+
{
1361+
FILETIME lastWriteTime{};
1362+
const auto styledString{ Json::writeString(wbuilder, ToJson()) };
1363+
til::io::write_utf8_string_to_file_atomic(settingsPath, styledString, &lastWriteTime);
13701364

1371-
_hash = _calculateHash(styledString, lastWriteTime);
1365+
_hash = _calculateHash(styledString, lastWriteTime);
13721366

1373-
// Persists the default terminal choice
1374-
// GH#10003 - Only do this if _currentDefaultTerminal was actually initialized.
1375-
if (_currentDefaultTerminal)
1367+
// Persists the default terminal choice
1368+
// GH#10003 - Only do this if _currentDefaultTerminal was actually initialized.
1369+
if (_currentDefaultTerminal)
1370+
{
1371+
DefaultTerminal::Current(_currentDefaultTerminal);
1372+
}
1373+
}
1374+
catch (...)
13761375
{
1377-
DefaultTerminal::Current(_currentDefaultTerminal);
1376+
LOG_CAUGHT_EXCEPTION();
1377+
_warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings);
1378+
return false;
13781379
}
1380+
return true;
13791381
}
13801382

13811383
#ifndef NDEBUG

‎src/inc/til/io.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,11 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
256256
// renaming one is (supposed to be) atomic.
257257
// Wait... "supposed to be"!? Well it's technically not always atomic,
258258
// but it's pretty darn close to it, so... better than nothing.
259-
std::filesystem::rename(tmpPath, resolvedPath);
259+
std::filesystem::rename(tmpPath, resolvedPath, ec);
260+
if (ec)
261+
{
262+
THROW_WIN32_MSG(ec.value(), "failed to write to file");
263+
}
260264
}
261265
} // io
262266
} // til

0 commit comments

Comments
 (0)