From 7d45da3b95f54f842549faac7b05b5674a03d999 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Jun 2021 17:39:03 +0200 Subject: [PATCH 1/4] Introduce a basic ApplicationState class --- doc/cascadia/AddASetting.md | 14 +- src/cascadia/TerminalApp/AppLogic.cpp | 85 +++----- src/cascadia/TerminalApp/AppLogic.h | 16 +- .../ApplicationState.cpp | 172 +++++++++++++++++ .../TerminalSettingsModel/ApplicationState.h | 65 +++++++ .../ApplicationState.idl | 13 ++ .../TerminalSettingsModel/CascadiaSettings.h | 3 +- .../CascadiaSettingsSerialization.cpp | 181 ++++-------------- .../TerminalSettingsModel/FileUtils.cpp | 126 ++++++++++++ .../TerminalSettingsModel/FileUtils.h | 11 ++ ...crosoft.Terminal.Settings.ModelLib.vcxproj | 9 + 11 files changed, 476 insertions(+), 219 deletions(-) create mode 100644 src/cascadia/TerminalSettingsModel/ApplicationState.cpp create mode 100644 src/cascadia/TerminalSettingsModel/ApplicationState.h create mode 100644 src/cascadia/TerminalSettingsModel/ApplicationState.idl create mode 100644 src/cascadia/TerminalSettingsModel/FileUtils.cpp create mode 100644 src/cascadia/TerminalSettingsModel/FileUtils.h diff --git a/doc/cascadia/AddASetting.md b/doc/cascadia/AddASetting.md index 16701771fea..2e6c99c211b 100644 --- a/doc/cascadia/AddASetting.md +++ b/doc/cascadia/AddASetting.md @@ -6,9 +6,9 @@ Adding a setting to Windows Terminal is fairly straightforward. This guide serve The Terminal Settings Model (`Microsoft.Terminal.Settings.Model`) is responsible for (de)serializing and exposing settings. -### `GETSET_SETTING` macro +### `INHERITABLE_SETTING` macro -The `GETSET_SETTING` macro can be used to implement inheritance for your new setting and store the setting in the settings model. It takes three parameters: +The `INHERITABLE_SETTING` macro can be used to implement inheritance for your new setting and store the setting in the settings model. It takes three parameters: - `type`: the type that the setting will be stored as - `name`: the name of the variable for storage - `defaultValue`: the value to use if the user does not define the setting anywhere @@ -20,7 +20,7 @@ This tutorial will add `CloseOnExitMode CloseOnExit` as a profile setting. 1. In `Profile.h`, declare/define the setting: ```c++ -GETSET_SETTING(CloseOnExitMode, CloseOnExit, CloseOnExitMode::Graceful) +INHERITABLE_SETTING(CloseOnExitMode, CloseOnExit, CloseOnExitMode::Graceful) ``` 2. In `Profile.idl`, expose the setting via WinRT: @@ -141,7 +141,7 @@ struct OpenSettingsArgs : public OpenSettingsArgsT OpenSettingsArgs() = default; // adds a getter/setter for your argument, and defines the json key - GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsFile); + WINRT_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsFile); static constexpr std::string_view TargetKey{ "target" }; public: @@ -213,9 +213,9 @@ Terminal-level settings are settings that affect a shell session. Generally, the - Declare the setting in `IControlSettings.idl` or `ICoreSettings.idl` (whichever is relevant to your setting). If your setting is an enum setting, declare the enum here instead of in the `TerminalSettingsModel` project. - In `TerminalSettings.h`, declare/define the setting... ```c++ -// The GETSET_PROPERTY macro declares/defines a getter setter for the setting. -// Like GETSET_SETTING, it takes in a type, name, and defaultValue. -GETSET_PROPERTY(bool, UseAcrylic, false); +// The WINRT_PROPERTY macro declares/defines a getter setter for the setting. +// Like INHERITABLE_SETTING, it takes in a type, name, and defaultValue. +WINRT_PROPERTY(bool, UseAcrylic, false); ``` - In `TerminalSettings.cpp`... - update `_ApplyProfileSettings` for profile settings diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index e6bf353d511..76ff21745c9 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -189,9 +189,7 @@ namespace winrt::TerminalApp::implementation } AppLogic::AppLogic() : - _dialogLock{}, - _loadedInitialSettings{ false }, - _settingsLoadedResult{ S_OK } + _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } } { // For your own sanity, it's better to do setup outside the ctor. // If you do any setup in the ctor that ends up throwing an exception, @@ -204,6 +202,13 @@ namespace winrt::TerminalApp::implementation // SetTitleBarContent _isElevated = _isUserAdmin(); _root = winrt::make_self(); + + _reloadSettings = std::make_shared>(_root->Dispatcher(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() { + if (auto self{ weakSelf.get() }) + { + self->_ReloadSettings(); + } + }); } // Method Description: @@ -859,59 +864,29 @@ namespace winrt::TerminalApp::implementation // - void AppLogic::_RegisterSettingsChange() { - // Get the containing folder. const std::filesystem::path settingsPath{ std::wstring_view{ CascadiaSettings::SettingsPath() } }; - const auto folder = settingsPath.parent_path(); - - _reader.create(folder.c_str(), - false, - wil::FolderChangeEvents::All, - [this, settingsPath](wil::FolderChangeEvent event, PCWSTR fileModified) { - // We want file modifications, AND when files are renamed to be - // settings.json. This second case will oftentimes happen with text - // editors, who will write a temp file, then rename it to be the - // actual file you wrote. So listen for that too. - if (!(event == wil::FolderChangeEvent::Modified || - event == wil::FolderChangeEvent::RenameNewName || - event == wil::FolderChangeEvent::Removed)) - { - return; - } - - std::filesystem::path modifiedFilePath = fileModified; - - // Getting basename (filename.ext) - const auto settingsBasename = settingsPath.filename(); - const auto modifiedBasename = modifiedFilePath.filename(); - - if (settingsBasename == modifiedBasename) - { - this->_DispatchReloadSettings(); - } - }); - } - - // Method Description: - // - Dispatches a settings reload with debounce. - // Text editors implement Save in a bunch of different ways, so - // this stops us from reloading too many times or too quickly. - fire_and_forget AppLogic::_DispatchReloadSettings() - { - if (_settingsReloadQueued.exchange(true)) - { - co_return; - } - - auto weakSelf = get_weak(); - - co_await winrt::resume_after(std::chrono::milliseconds(100)); - co_await winrt::resume_foreground(_root->Dispatcher()); - - if (auto self{ weakSelf.get() }) - { - _ReloadSettings(); - _settingsReloadQueued.store(false); - } + const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; + + _reader.create( + settingsPath.parent_path().c_str(), + false, + // We want file modifications, AND when files are renamed to be + // settings.json. This second case will oftentimes happen with text + // editors, who will write a temp file, then rename it to be the + // actual file you wrote. So listen for that too. + wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, + [this, settingsBasename = settingsPath.filename(), stateBasename = statePath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { + const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); + + if (modifiedBasename == settingsBasename) + { + _reloadSettings->Run(); + } + else if (modifiedBasename == stateBasename) + { + _reloadState(); + } + }); } void AppLogic::_ApplyLanguageSettingChange() noexcept diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h index bb02b7298f7..dfbe27a4402 100644 --- a/src/cascadia/TerminalApp/AppLogic.h +++ b/src/cascadia/TerminalApp/AppLogic.h @@ -7,7 +7,9 @@ #include "FindTargetWindowResult.g.h" #include "TerminalPage.h" #include "Jumplist.h" -#include "../../cascadia/inc/cppwinrt_utils.h" + +#include +#include #ifdef UNIT_TESTING // fwdecl unittest classes @@ -111,17 +113,15 @@ namespace winrt::TerminalApp::implementation Microsoft::Terminal::Settings::Model::CascadiaSettings _settings{ nullptr }; - HRESULT _settingsLoadedResult; - winrt::hstring _settingsLoadExceptionText{}; - - bool _loadedInitialSettings; - wil::unique_folder_change_reader_nothrow _reader; + std::shared_ptr> _reloadSettings; + til::throttled_func_trailing<> _reloadState; + winrt::hstring _settingsLoadExceptionText; + HRESULT _settingsLoadedResult = S_OK; + bool _loadedInitialSettings = false; std::shared_mutex _dialogLock; - std::atomic _settingsReloadQueued{ false }; - ::TerminalApp::AppCommandlineArgs _appArgs; ::TerminalApp::AppCommandlineArgs _settingsAppArgs; static TerminalApp::FindTargetWindowResult _doFindTargetWindow(winrt::array_view args, diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp new file mode 100644 index 00000000000..d6049d86d4a --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -0,0 +1,172 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" +#include "ApplicationState.h" +#include "CascadiaSettings.h" +#include "ApplicationState.g.cpp" + +#include "JsonUtils.h" +#include "FileUtils.h" + +namespace Microsoft::Terminal::Settings::Model::JsonUtils +{ + // This trait exists in order to serialize the std::unordered_set for GeneratedProfiles. + template + struct ConversionTrait> + { + std::unordered_set FromJson(const Json::Value& json) const + { + ConversionTrait trait; + std::unordered_set val; + val.reserve(json.size()); + + for (const auto& element : json) + { + val.emplace(trait.FromJson(element)); + } + + return val; + } + + bool CanConvert(const Json::Value& json) const + { + ConversionTrait trait; + return json.isArray() && std::all_of(json.begin(), json.end(), [trait](const auto& json) -> bool { return trait.CanConvert(json); }); + } + + Json::Value ToJson(const std::unordered_set& val) + { + ConversionTrait trait; + Json::Value json{ Json::arrayValue }; + + for (const auto& key : val) + { + json.append(trait.ToJson(key)); + } + + return json; + } + + std::string TypeDescription() const + { + return fmt::format("std::unordered_set<{}>", ConversionTrait{}.TypeDescription()); + } + }; +} + +using namespace ::Microsoft::Terminal::Settings::Model; + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + // Returns the application-global ApplicationState object. + Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() + { + static auto state = winrt::make_self(GetBaseSettingsPath() / L"state.json"); + return *state; + } + + ApplicationState::ApplicationState(std::filesystem::path path) noexcept : + _path{ std::move(path) }, + _throttler{ std::chrono::seconds(1), [this]() { _write(); } } + { + _read(); + } + + // The destructor ensures that the last write is flushed to disk before returning. + ApplicationState::~ApplicationState() + { + // This will ensure that we not just cancel the last outstanding timer, + // but instead force it run as soon as possible and wait for it to complete. + _throttler.flush(); + } + + // Re-read the state.json from disk. + void ApplicationState::Reload() const noexcept + { + _read(); + } + + // Returns the state.json path on the disk. + winrt::hstring ApplicationState::FilePath() const noexcept + { + return winrt::hstring{ _path.wstring() }; + } + + // Generate all getter/setters +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ + type ApplicationState::name() const noexcept \ + { \ + const auto state = _state.lock_shared(); \ + const auto& value = state->name; \ + return value ? *value : type{ __VA_ARGS__ }; \ + } \ + \ + void ApplicationState::name(const type& value) noexcept \ + { \ + { \ + auto state = _state.lock(); \ + state->name.emplace(value); \ + } \ + \ + _throttler(); \ + } + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + + // Deserializes the state.json at _path into this ApplicationState. + // * *ANY* errors during app state will result in the creation of a new empty state. + // * *ANY* errors during runtime will result in changes being partially ignored. + // * Doesn't acquire any locks - may only be called by ApplicationState's constructor. + void ApplicationState::_read() const noexcept + try + { + const auto data = ReadUTF8FileIfExists(_path).value_or(std::string{}); + if (data.empty()) + { + return; + } + + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + + Json::Value root; + if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + + auto state = _state.lock(); + // GetValueForKey() comes in two variants: + // * take a std::optional reference + // * return std::optional by value + // At the time of writing the former version skips missing fields in the json, + // but we want to explicitly clear state fields that were removed from state.json. +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + } + CATCH_LOG() + + // Serialized this ApplicationState (in `context`) into the state.json at _path. + // * Errors are only logged. + // * _state->_writeScheduled is set to false, signaling our + // setters that _synchronize() needs to be called again. + void ApplicationState::_write() const noexcept + try + { + Json::Value root{ Json::objectValue }; + + { + auto state = _state.lock_shared(); +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + } + + Json::StreamWriterBuilder wbuilder; + const auto content = Json::writeString(wbuilder, root); + WriteUTF8FileAtomic(_path, content); + } + CATCH_LOG() +} diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h new file mode 100644 index 00000000000..d77f2cadd4b --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -0,0 +1,65 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- ApplicationState.h + +--*/ +#pragma once + +#include "ApplicationState.g.h" + +#include +#include +#include + +// This macro generates all getters and setters for ApplicationState. +// It provides X with the following arguments: +// (type, function name, JSON key, ...variadic construction arguments) +#define MTSM_APPLICATION_STATE_FIELDS(X) \ + X(std::unordered_set, GeneratedProfiles, "generatedProfiles") + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + struct ApplicationState : ApplicationStateT + { + static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); + + ApplicationState(std::filesystem::path path) noexcept; + ~ApplicationState(); + + // Methods + void Reload() const noexcept; + + // General getters/setters + winrt::hstring FilePath() const noexcept; + + // State getters/setters +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ + type name() const noexcept; \ + void name(const type& value) noexcept; + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + + private: + struct state_t + { +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + }; + + void _write() const noexcept; + void _read() const noexcept; + + std::filesystem::path _path; + til::shared_mutex _state; + til::throttled_func_trailing<> _throttler; + }; +} + +namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation +{ + BASIC_FACTORY(ApplicationState); +} diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.idl b/src/cascadia/TerminalSettingsModel/ApplicationState.idl new file mode 100644 index 00000000000..8f5eed84ed4 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.idl @@ -0,0 +1,13 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace Microsoft.Terminal.Settings.Model +{ + [default_interface] runtimeclass ApplicationState { + static ApplicationState SharedInstance(); + + void Reload(); + + String FilePath { get; }; + } +} diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index a4e328ebedc..e527187b658 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -147,9 +147,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unordered_set _AccumulateJsonFilesInDirectory(const std::wstring_view directory); void _ParseAndLayerFragmentFiles(const std::unordered_set files, const winrt::hstring source); - static void _WriteSettings(std::string_view content, const hstring filepath); + static const std::filesystem::path& _SettingsPath(); static std::optional _ReadUserSettings(); - static std::optional _ReadFile(HANDLE hFile); std::optional _GetProfileGuidByName(const hstring) const; std::optional _GetProfileGuidByIndex(std::optional index) const; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 0f6ad7593ef..faa1a9db477 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -7,8 +7,6 @@ #include #include -#include - // defaults.h is a file containing the default json settings in a std::string_view #include "defaults.h" #include "defaults-universal.h" @@ -17,12 +15,15 @@ // Both defaults.h and userDefaults.h are generated at build time into the // "Generated Files" directory. +#include "ApplicationState.h" +#include "FileUtils.h" + using namespace winrt::Microsoft::Terminal::Settings::Model::implementation; using namespace ::Microsoft::Console; +using namespace ::Microsoft::Terminal::Settings::Model; static constexpr std::wstring_view SettingsFilename{ L"settings.json" }; static constexpr std::wstring_view LegacySettingsFilename{ L"profiles.json" }; -static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; static constexpr std::wstring_view DefaultsFilename{ L"defaults.json" }; @@ -40,7 +41,6 @@ static constexpr std::string_view GuidKey{ "guid" }; static constexpr std::string_view DisabledProfileSourcesKey{ "disabledProfileSources" }; -static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; static constexpr std::string_view SettingsSchemaFragment{ "\n" R"( "$schema": "https://aka.ms/terminal-profiles-schema")" }; @@ -234,7 +234,7 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: try { - _WriteSettings(resultPtr->_userSettingsString, CascadiaSettings::SettingsPath()); + WriteUTF8FileAtomic(_SettingsPath(), resultPtr->_userSettingsString); } catch (...) { @@ -491,23 +491,11 @@ std::unordered_set CascadiaSettings::_AccumulateJsonFilesInDirector { if (fragmentExt.path().extension() == jsonExtension) { - wil::unique_hfile hFile{ CreateFileW(fragmentExt.path().c_str(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - nullptr) }; - - if (!hFile) - { - LOG_LAST_ERROR(); - } - else + try { - const auto fileData = _ReadFile(hFile.get()).value(); - jsonFiles.emplace(fileData); + jsonFiles.emplace(ReadUTF8File(fragmentExt.path())); } + CATCH_LOG(); } } return jsonFiles; @@ -637,13 +625,8 @@ void CascadiaSettings::_ParseJsonString(std::string_view fileData, const bool is Json::Value CascadiaSettings::_ParseUtf8JsonString(std::string_view fileData) { Json::Value result; - // Ignore UTF-8 BOM - auto actualDataStart = fileData.data(); + const auto actualDataStart = fileData.data(); const auto actualDataEnd = fileData.data() + fileData.size(); - if (fileData.compare(0, Utf8Bom.size(), Utf8Bom) == 0) - { - actualDataStart += Utf8Bom.size(); - } std::string errs; // This string will receive any error text from failing to parse. std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; @@ -693,8 +676,7 @@ bool CascadiaSettings::_PrependSchemaDirective() // them into the user's settings at the end of the list of profiles. // - Does not reformat the user's settings file. // - Does not write the file! Only modifies in-place the _userSettingsString -// member. Callers should make sure to call -// _WriteSettings(_userSettingsString) to make sure to persist these changes! +// member. Callers should make sure to persist these changes (see WriteSettingsToDisk). // - Assumes that the `profiles` object is at an indentation of 4 spaces, and // therefore each profile should be indented 8 spaces. If the user's settings // have a different indentation, we'll still insert valid json, it'll just be @@ -1056,28 +1038,15 @@ winrt::com_ptr CascadiaSettings::_FindMatchingColorScheme(const Jso } // Method Description: -// - Writes the given content in UTF-8 to a settings file using the Win32 APIS's. -// Will overwrite any existing content in the file. +// - Returns the path of the settings.json file. // Arguments: -// - content: the given string of content to write to the file. -// Return Value: // - -// This can throw an exception if we fail to open the file for writing, or we -// fail to write the file -void CascadiaSettings::_WriteSettings(const std::string_view content, const hstring filepath) +// Return Value: +// - Returns a path in 80% of cases. I measured! +const std::filesystem::path& CascadiaSettings::_SettingsPath() { - wil::unique_hfile hOut{ CreateFileW(filepath.c_str(), - GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - CREATE_ALWAYS, - FILE_ATTRIBUTE_NORMAL, - nullptr) }; - if (!hOut) - { - THROW_LAST_ERROR(); - } - THROW_LAST_ERROR_IF(!WriteFile(hOut.get(), content.data(), gsl::narrow(content.size()), nullptr, nullptr)); + static const auto path = GetBaseSettingsPath() / SettingsFilename; + return path; } // Method Description: @@ -1091,92 +1060,31 @@ void CascadiaSettings::_WriteSettings(const std::string_view content, const hstr // from reading the file std::optional CascadiaSettings::_ReadUserSettings() { - const auto pathToSettingsFile{ CascadiaSettings::SettingsPath() }; - wil::unique_hfile hFile{ CreateFileW(pathToSettingsFile.c_str(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - nullptr) }; - - if (!hFile) + const auto pathToSettingsFile = _SettingsPath(); + auto data = ReadUTF8FileIfExists(pathToSettingsFile); + + if (!data) { // GH#5186 - We moved from profiles.json to settings.json; we want to // migrate any file we find. We're using MoveFile in case their settings.json // is a symbolic link. - std::filesystem::path pathToLegacySettingsFile{ std::wstring_view{ pathToSettingsFile } }; + auto pathToLegacySettingsFile = pathToSettingsFile; pathToLegacySettingsFile.replace_filename(LegacySettingsFilename); - wil::unique_hfile hLegacyFile{ CreateFileW(pathToLegacySettingsFile.c_str(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - nullptr) }; + data = ReadUTF8FileIfExists(pathToLegacySettingsFile); - if (hLegacyFile) + if (data) { - // Close the file handle, move it, and re-open the file in its new location. - hLegacyFile.reset(); - // Note: We're unsure if this is unsafe. Theoretically it's possible // that two instances of the app will try and move the settings file // simultaneously. We don't know what might happen in that scenario, // but we're also not sure how to safely lock the file to prevent // that from occurring. - THROW_LAST_ERROR_IF(!MoveFile(pathToLegacySettingsFile.c_str(), - pathToSettingsFile.c_str())); - - hFile.reset(CreateFileW(pathToSettingsFile.c_str(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - nullptr)); - - // hFile shouldn't be INVALID. That's unexpected - We just moved the - // file, we should be able to open it. Throw the error so we can get - // some information here. - THROW_LAST_ERROR_IF(!hFile); - } - else - { - // If the roaming file didn't exist, and the local file doesn't exist, - // that's fine. Just log the error and return nullopt - we'll - // create the defaults. - LOG_LAST_ERROR(); - return std::nullopt; + std::filesystem::rename(pathToLegacySettingsFile, pathToSettingsFile); } } - return _ReadFile(hFile.get()); -} - -// Method Description: -// - Reads the content in UTF-8 encoding of the given file using the Win32 APIs -// Arguments: -// - -// Return Value: -// - an optional with the content of the file if we were able to read it. If we -// fail to read it, this can throw an exception from reading the file -std::optional CascadiaSettings::_ReadFile(HANDLE hFile) -{ - // fileSize is in bytes - const auto fileSize = GetFileSize(hFile, nullptr); - THROW_LAST_ERROR_IF(fileSize == INVALID_FILE_SIZE); - - auto utf8buffer = std::make_unique(fileSize); - - DWORD bytesRead = 0; - THROW_LAST_ERROR_IF(!ReadFile(hFile, utf8buffer.get(), fileSize, &bytesRead, nullptr)); - - // convert buffer to UTF-8 string - std::string utf8string(utf8buffer.get(), fileSize); - - return { utf8string }; + return data; } // function Description: @@ -1191,23 +1099,7 @@ std::optional CascadiaSettings::_ReadFile(HANDLE hFile) // - the full path to the settings file winrt::hstring CascadiaSettings::SettingsPath() { - wil::unique_cotaskmem_string localAppDataFolder; - // KF_FLAG_FORCE_APP_DATA_REDIRECTION, when engaged, causes SHGet... to return - // the new AppModel paths (Packages/xxx/RoamingState, etc.) for standard path requests. - // Using this flag allows us to avoid Windows.Storage.ApplicationData completely. - THROW_IF_FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppData, KF_FLAG_FORCE_APP_DATA_REDIRECTION, nullptr, &localAppDataFolder)); - - std::filesystem::path parentDirectoryForSettingsFile{ localAppDataFolder.get() }; - - if (!IsPackaged()) - { - parentDirectoryForSettingsFile /= UnpackagedSettingsFolderName; - } - - // Create the directory if it doesn't exist - std::filesystem::create_directories(parentDirectoryForSettingsFile); - - return winrt::hstring{ (parentDirectoryForSettingsFile / SettingsFilename).wstring() }; + return winrt::hstring{ _SettingsPath().wstring() }; } winrt::hstring CascadiaSettings::DefaultSettingsPath() @@ -1222,15 +1114,12 @@ winrt::hstring CascadiaSettings::DefaultSettingsPath() // directory as the exe, that will work for unpackaged scenarios as well. So // let's try that. - HMODULE hModule = GetModuleHandle(nullptr); - THROW_LAST_ERROR_IF(hModule == nullptr); - std::wstring exePathString; - THROW_IF_FAILED(wil::GetModuleFileNameW(hModule, exePathString)); + THROW_IF_FAILED(wil::GetModuleFileNameW(nullptr, exePathString)); - const std::filesystem::path exePath{ exePathString }; - const std::filesystem::path rootDir = exePath.parent_path(); - return winrt::hstring{ (rootDir / DefaultsFilename).wstring() }; + std::filesystem::path path{ exePathString }; + path.replace_filename(DefaultsFilename); + return winrt::hstring{ path.wstring() }; } // Function Description: @@ -1275,15 +1164,13 @@ const Json::Value& CascadiaSettings::_GetDisabledProfileSourcesJsonObject(const // - void CascadiaSettings::WriteSettingsToDisk() const { - const auto settingsPath{ CascadiaSettings::SettingsPath() }; + const auto settingsPath = _SettingsPath(); try { // create a timestamped backup file - const auto clock{ std::chrono::system_clock() }; - const auto timeStamp{ clock.to_time_t(clock.now()) }; - const winrt::hstring backupSettingsPath{ fmt::format(L"{}.{:%Y-%m-%dT%H-%M-%S}.backup", settingsPath, fmt::localtime(timeStamp)) }; - _WriteSettings(_userSettingsString, backupSettingsPath); + const auto backupSettingsPath = fmt::format(L"{}.{:%Y-%m-%dT%H-%M-%S}.backup", settingsPath.wstring(), fmt::localtime(std::time(nullptr))); + WriteUTF8File(backupSettingsPath, _userSettingsString); } CATCH_LOG(); @@ -1293,7 +1180,7 @@ void CascadiaSettings::WriteSettingsToDisk() const wbuilder.settings_["enableYAMLCompatibility"] = true; // suppress spaces around colons const auto styledString{ Json::writeString(wbuilder, ToJson()) }; - _WriteSettings(styledString, settingsPath); + WriteUTF8FileAtomic(settingsPath, styledString); // Persists the default terminal choice // diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp new file mode 100644 index 00000000000..eded35e6490 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -0,0 +1,126 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" +#include "FileUtils.h" + +#include +#include +#include + +static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; +static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; + +namespace Microsoft::Terminal::Settings::Model +{ + std::filesystem::path GetBaseSettingsPath() + { + static std::filesystem::path baseSettingsPath = []() { + wil::unique_cotaskmem_string localAppDataFolder; + // KF_FLAG_FORCE_APP_DATA_REDIRECTION, when engaged, causes SHGet... to return + // the new AppModel paths (Packages/xxx/RoamingState, etc.) for standard path requests. + // Using this flag allows us to avoid Windows.Storage.ApplicationData completely. + THROW_IF_FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppData, KF_FLAG_FORCE_APP_DATA_REDIRECTION, nullptr, &localAppDataFolder)); + + std::filesystem::path parentDirectoryForSettingsFile{ localAppDataFolder.get() }; + + if (!IsPackaged()) + { + parentDirectoryForSettingsFile /= UnpackagedSettingsFolderName; + } + + // Create the directory if it doesn't exist + std::filesystem::create_directories(parentDirectoryForSettingsFile); + + return parentDirectoryForSettingsFile; + }(); + return baseSettingsPath; + } + + std::string ReadUTF8File(const std::filesystem::path& path) + { + // From some casual observations we can determine that: + // * ReadFile() always returns the requested amount of data (unless the file is smaller) + // * It's unlikely that the file was changed between GetFileSize() and ReadFile() + // -> Lets add a retry-loop just in case, to not fail if the file size changed. + for (int i = 0; i < 3; ++i) { + wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; + THROW_LAST_ERROR_IF(!file); + + const auto fileSize = GetFileSize(file.get(), nullptr); + THROW_LAST_ERROR_IF(fileSize == INVALID_FILE_SIZE); + + // By making our buffer just slightly larger we can detect if + // the file size changed and we've failed to read the full file. + std::string buffer(static_cast(fileSize) + 1, '\0'); + DWORD bytesRead = 0; + THROW_LAST_ERROR_IF(!ReadFile(file.get(), buffer.data(), gsl::narrow(buffer.size()), &bytesRead, nullptr)); + + // This implementation isn't atomic as it's not trivially possible to + // atomically read the entire file without breaking workflow of our users. + // For instance locking the file would conflict with users who have the file open in an editor. + // We'll just throw in case where the file size changed as that's likely an error. + if (bytesRead != fileSize) + { + // This continue is unlikely to be hit (see the prior for loop comment). + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + continue; + } + + // As mentioned before our buffer was allocated oversized. + buffer.resize(bytesRead); + + if (til::starts_with(buffer, Utf8Bom)) + { + buffer.erase(0, Utf8Bom.size()); + } + + return buffer; + } + + THROW_WIN32_MSG(ERROR_READ_FAULT, "file size changed while reading"); + } + + // Returns an empty optional (aka std::nullopt), if the file couldn't be opened. + // Use THROW_LAST_ERROR() if you want to fail in that case. + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path) + { + try + { + return { ReadUTF8File(path) }; + } + catch (const wil::ResultException& exception) + { + if (exception.GetErrorCode() == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + { + return {}; + } + + throw; + } + } + + void WriteUTF8File(const std::filesystem::path& path, const std::string_view content) + { + wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; + THROW_LAST_ERROR_IF(!file); + + const auto fileSize = gsl::narrow(content.size()); + DWORD bytesWritten = 0; + THROW_LAST_ERROR_IF(!WriteFile(file.get(), content.data(), fileSize, &bytesWritten, nullptr)); + + if (bytesWritten != fileSize) + { + THROW_WIN32_MSG(ERROR_WRITE_FAULT, "failed to write whole file"); + } + } + + void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view content) + { + auto tmpPath = path; + tmpPath += L".tmp"; + + WriteUTF8File(tmpPath, content); + std::filesystem::rename(tmpPath, path); + } +} diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.h b/src/cascadia/TerminalSettingsModel/FileUtils.h new file mode 100644 index 00000000000..d2e2eb53c7d --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/FileUtils.h @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace Microsoft::Terminal::Settings::Model +{ + std::filesystem::path GetBaseSettingsPath(); + std::string ReadUTF8File(const std::filesystem::path& path); + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path); + void WriteUTF8File(const std::filesystem::path& path, const std::string_view content); + void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view content); +} diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index c2b645bd02b..4499f305eb9 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -32,6 +32,9 @@ ActionMap.idl + + ApplicationState.idl + CascadiaSettings.idl @@ -42,6 +45,7 @@ Command.idl + GlobalAppSettings.idl @@ -97,6 +101,9 @@ ActionMap.idl + + ApplicationState.idl + CascadiaSettings.idl @@ -110,6 +117,7 @@ Command.idl + GlobalAppSettings.idl @@ -141,6 +149,7 @@ + From 48d12b543d3d1fe9a6c87165b6197d21330031c3 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Jun 2021 17:44:29 +0200 Subject: [PATCH 2/4] Fix spell check --- .github/actions/spelling/expect/expect.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 6ddbe8c3f24..00fe5ab1e10 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1443,6 +1443,7 @@ MSVCRTD MSVS msys msysgit +MTSM mui Mul multiline @@ -2416,7 +2417,6 @@ uapadmin UAX ubuntu ucd -ucd ucdxml uch UCHAR @@ -2780,7 +2780,6 @@ xml xmlns xor xorg -xorg Xpath XPosition XResource From 2c12673d29f9433e3141d635ac17ae63c21aad46 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Jun 2021 17:48:42 +0200 Subject: [PATCH 3/4] Fix formatting check --- src/cascadia/TerminalSettingsModel/FileUtils.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index eded35e6490..c186eef8c05 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -43,7 +43,8 @@ namespace Microsoft::Terminal::Settings::Model // * ReadFile() always returns the requested amount of data (unless the file is smaller) // * It's unlikely that the file was changed between GetFileSize() and ReadFile() // -> Lets add a retry-loop just in case, to not fail if the file size changed. - for (int i = 0; i < 3; ++i) { + for (int i = 0; i < 3; ++i) + { wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; THROW_LAST_ERROR_IF(!file); From 08ef30b0a14cb5f9ac915d36e8d9f72484763994 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 30 Jun 2021 01:26:36 +0200 Subject: [PATCH 4/4] Address reviewer comments & improve docs --- .../ApplicationState.cpp | 13 +++++---- .../TerminalSettingsModel/ApplicationState.h | 4 +++ .../CascadiaSettingsSerialization.cpp | 26 +---------------- .../TerminalSettingsModel/FileUtils.cpp | 28 +++++++++++++------ 4 files changed, 31 insertions(+), 40 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index d6049d86d4a..0984a5330cf 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -9,6 +9,8 @@ #include "JsonUtils.h" #include "FileUtils.h" +constexpr std::wstring_view stateFileName{ L"state.json" }; + namespace Microsoft::Terminal::Settings::Model::JsonUtils { // This trait exists in order to serialize the std::unordered_set for GeneratedProfiles. @@ -50,7 +52,7 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils std::string TypeDescription() const { - return fmt::format("std::unordered_set<{}>", ConversionTrait{}.TypeDescription()); + return fmt::format("{}[]", ConversionTrait{}.TypeDescription()); } }; } @@ -62,7 +64,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { - static auto state = winrt::make_self(GetBaseSettingsPath() / L"state.json"); + static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); return *state; } @@ -77,7 +79,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::~ApplicationState() { // This will ensure that we not just cancel the last outstanding timer, - // but instead force it run as soon as possible and wait for it to complete. + // but instead force it to run as soon as possible and wait for it to complete. _throttler.flush(); } @@ -115,9 +117,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation #undef MTSM_APPLICATION_STATE_GEN // Deserializes the state.json at _path into this ApplicationState. - // * *ANY* errors during app state will result in the creation of a new empty state. - // * *ANY* errors during runtime will result in changes being partially ignored. - // * Doesn't acquire any locks - may only be called by ApplicationState's constructor. + // * ANY errors during app state will result in the creation of a new empty state. + // * ANY errors during runtime will result in changes being partially ignored. void ApplicationState::_read() const noexcept try { diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index d77f2cadd4b..90320f0e2a4 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -5,6 +5,10 @@ Licensed under the MIT license. Module Name: - ApplicationState.h +Abstract: +- If the CascadiaSettings class were AppData, then this class would be LocalAppData. + Put anything in here that you wouldn't want to be stored next to user-editable settings. +- Modify ApplicationState.idl and MTSM_APPLICATION_STATE_FIELDS to add new fields. --*/ #pragma once diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index faa1a9db477..3b4aa1db639 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -1060,31 +1060,7 @@ const std::filesystem::path& CascadiaSettings::_SettingsPath() // from reading the file std::optional CascadiaSettings::_ReadUserSettings() { - const auto pathToSettingsFile = _SettingsPath(); - auto data = ReadUTF8FileIfExists(pathToSettingsFile); - - if (!data) - { - // GH#5186 - We moved from profiles.json to settings.json; we want to - // migrate any file we find. We're using MoveFile in case their settings.json - // is a symbolic link. - auto pathToLegacySettingsFile = pathToSettingsFile; - pathToLegacySettingsFile.replace_filename(LegacySettingsFilename); - - data = ReadUTF8FileIfExists(pathToLegacySettingsFile); - - if (data) - { - // Note: We're unsure if this is unsafe. Theoretically it's possible - // that two instances of the app will try and move the settings file - // simultaneously. We don't know what might happen in that scenario, - // but we're also not sure how to safely lock the file to prevent - // that from occurring. - std::filesystem::rename(pathToLegacySettingsFile, pathToSettingsFile); - } - } - - return data; + return ReadUTF8FileIfExists(_SettingsPath()); } // function Description: diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index c186eef8c05..b952f6a3d3c 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -13,6 +13,8 @@ static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Wi namespace Microsoft::Terminal::Settings::Model { + // Returns a path like C:\Users\\AppData\Local\Packages\\LocalState + // You can put your settings.json or state.json in this directory. std::filesystem::path GetBaseSettingsPath() { static std::filesystem::path baseSettingsPath = []() { @@ -37,12 +39,14 @@ namespace Microsoft::Terminal::Settings::Model return baseSettingsPath; } + // Tries to read a file somewhat atomically without locking it. + // Strips the UTF8 BOM if it exists. std::string ReadUTF8File(const std::filesystem::path& path) { // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) // * It's unlikely that the file was changed between GetFileSize() and ReadFile() - // -> Lets add a retry-loop just in case, to not fail if the file size changed. + // -> Lets add a retry-loop just in case, to not fail if the file size changed while reading. for (int i = 0; i < 3; ++i) { wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; @@ -55,12 +59,11 @@ namespace Microsoft::Terminal::Settings::Model // the file size changed and we've failed to read the full file. std::string buffer(static_cast(fileSize) + 1, '\0'); DWORD bytesRead = 0; - THROW_LAST_ERROR_IF(!ReadFile(file.get(), buffer.data(), gsl::narrow(buffer.size()), &bytesRead, nullptr)); + THROW_IF_WIN32_BOOL_FALSE(ReadFile(file.get(), buffer.data(), gsl::narrow(buffer.size()), &bytesRead, nullptr)); - // This implementation isn't atomic as it's not trivially possible to - // atomically read the entire file without breaking workflow of our users. - // For instance locking the file would conflict with users who have the file open in an editor. - // We'll just throw in case where the file size changed as that's likely an error. + // This implementation isn't atomic as we'd need to use an exclusive file lock. + // But this would be annoying for users as it forces them to close the file in their editor. + // The next best alternative is to at least try to detect file changes and retry the read. if (bytesRead != fileSize) { // This continue is unlikely to be hit (see the prior for loop comment). @@ -73,6 +76,9 @@ namespace Microsoft::Terminal::Settings::Model if (til::starts_with(buffer, Utf8Bom)) { + // Yeah this memmove()s the entire content. + // But I don't really want to deal with UTF8 BOMs any more than necessary, + // as basically not a single editor writes a BOM for UTF8. buffer.erase(0, Utf8Bom.size()); } @@ -82,8 +88,7 @@ namespace Microsoft::Terminal::Settings::Model THROW_WIN32_MSG(ERROR_READ_FAULT, "file size changed while reading"); } - // Returns an empty optional (aka std::nullopt), if the file couldn't be opened. - // Use THROW_LAST_ERROR() if you want to fail in that case. + // Same as ReadUTF8File, but returns an empty optional, if the file couldn't be opened. std::optional ReadUTF8FileIfExists(const std::filesystem::path& path) { try @@ -108,7 +113,7 @@ namespace Microsoft::Terminal::Settings::Model const auto fileSize = gsl::narrow(content.size()); DWORD bytesWritten = 0; - THROW_LAST_ERROR_IF(!WriteFile(file.get(), content.data(), fileSize, &bytesWritten, nullptr)); + THROW_IF_WIN32_BOOL_FALSE(WriteFile(file.get(), content.data(), fileSize, &bytesWritten, nullptr)); if (bytesWritten != fileSize) { @@ -121,7 +126,12 @@ namespace Microsoft::Terminal::Settings::Model auto tmpPath = path; tmpPath += L".tmp"; + // Writing to a file isn't atomic, but... WriteUTF8File(tmpPath, content); + + // 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, path); } }