From ccf550bf5504b6c2483c117b165c44f6fd11f00f Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 15 Aug 2022 15:35:51 +0200 Subject: [PATCH] Call UpdateJumplist only if the settings changed --- src/cascadia/TerminalApp/AppLogic.cpp | 27 +++++++++++++++++-- src/cascadia/TerminalApp/AppLogic.h | 1 + .../TerminalSettingsModel/ApplicationState.h | 1 + .../ApplicationState.idl | 13 ++++----- .../CascadiaSettings.cpp | 5 ++++ .../TerminalSettingsModel/CascadiaSettings.h | 5 +++- .../CascadiaSettings.idl | 4 ++- .../CascadiaSettingsSerialization.cpp | 25 ++++++++++++++--- .../TerminalSettingsModel/FileUtils.cpp | 27 ++++++++++++------- .../TerminalSettingsModel/FileUtils.h | 8 +++--- 10 files changed, 87 insertions(+), 29 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 591bc079016..17421c80ff0 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -297,6 +297,11 @@ namespace winrt::TerminalApp::implementation { _root->SetFullscreen(true); } + + // Both LoadSettings and ReloadSettings are supposed to call this function, + // but LoadSettings skips it, so that the UI starts up faster. + // Now that the UI is present we can do them with a less significant UX impact. + _ProcessLazySettingsChanges(); }); _root->Create(); @@ -886,8 +891,27 @@ namespace winrt::TerminalApp::implementation // Register for directory change notification. _RegisterSettingsChange(); + } + + // Call this function after loading your _settings. + // It handles any CPU intensive settings updates (like updating the Jumplist) + // which should thus only occur if the settings file actually changed. + void AppLogic::_ProcessLazySettingsChanges() + { + const auto hash = _settings.Hash(); + const auto applicationState = ApplicationState::SharedInstance(); + const auto cachedHash = applicationState.SettingsHash(); + + // The hash might be empty if LoadAll() failed and we're dealing with the defaults settings object. + // In that case we can just wait until the user fixed their settings or CascadiaSettings fixed + // itself and either will soon trigger a settings reload. + if (hash.empty() || hash == cachedHash) + { + return; + } Jumplist::UpdateJumplist(_settings); + applicationState.SettingsHash(hash); } // Method Description: @@ -1058,8 +1082,7 @@ namespace winrt::TerminalApp::implementation _ApplyLanguageSettingChange(); _RefreshThemeRoutine(); _ApplyStartupTaskStateChange(); - - Jumplist::UpdateJumplist(_settings); + _ProcessLazySettingsChanges(); _SettingsChangedHandlers(*this, nullptr); } diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h index 6b715e0c7bb..f4abe8254f2 100644 --- a/src/cascadia/TerminalApp/AppLogic.h +++ b/src/cascadia/TerminalApp/AppLogic.h @@ -192,6 +192,7 @@ namespace winrt::TerminalApp::implementation void _OnLoaded(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs); [[nodiscard]] HRESULT _TryLoadSettings() noexcept; + void _ProcessLazySettingsChanges(); void _RegisterSettingsChange(); fire_and_forget _DispatchReloadSettings(); void _ReloadSettings(); diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index a5e607e0b17..91183bad076 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -35,6 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // It provides X with the following arguments: // (source, type, function name, JSON key, ...variadic construction arguments) #define MTSM_APPLICATION_STATE_FIELDS(X) \ + X(FileSource::Shared, winrt::hstring, SettingsHash, "settingsHash") \ X(FileSource::Shared, std::unordered_set, GeneratedProfiles, "generatedProfiles") \ X(FileSource::Local, Windows::Foundation::Collections::IVector, PersistedWindowLayouts, "persistedWindowLayouts") \ X(FileSource::Shared, Windows::Foundation::Collections::IVector, RecentCommands, "recentCommands") \ diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.idl b/src/cascadia/TerminalSettingsModel/ApplicationState.idl index 13ea22bf0d3..e16daefe336 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.idl +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.idl @@ -33,13 +33,10 @@ namespace Microsoft.Terminal.Settings.Model Boolean IsStatePath(String filename); - Windows.Foundation.Collections.IVector PersistedWindowLayouts { get; set; }; - - Windows.Foundation.Collections.IVector RecentCommands { get; set; }; - - Windows.Foundation.Collections.IVector DismissedMessages { get; set; }; - - Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; - + String SettingsHash; + Windows.Foundation.Collections.IVector PersistedWindowLayouts; + Windows.Foundation.Collections.IVector RecentCommands; + Windows.Foundation.Collections.IVector DismissedMessages; + Windows.Foundation.Collections.IVector AllowedCommandlines; } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index a34183d80b0..9712c6e51cf 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -43,6 +43,11 @@ winrt::com_ptr Model::implementation::CreateChild(const winrt::com_ptr< return profile; } +winrt::hstring CascadiaSettings::Hash() const noexcept +{ + return _hash; +} + Model::CascadiaSettings CascadiaSettings::Copy() const { const auto settings{ winrt::make_self() }; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index c6141df700c..85e594f9656 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -113,12 +113,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation explicit CascadiaSettings(SettingsLoader&& loader); // user settings + winrt::hstring Hash() const noexcept; Model::CascadiaSettings Copy() const; Model::GlobalAppSettings GlobalSettings() const; winrt::Windows::Foundation::Collections::IObservableVector AllProfiles() const noexcept; winrt::Windows::Foundation::Collections::IObservableVector ActiveProfiles() const noexcept; Model::ActionMap ActionMap() const noexcept; - void WriteSettingsToDisk() const; + void WriteSettingsToDisk(); Json::Value ToJson() const; Model::Profile ProfileDefaults() const; Model::Profile CreateNewProfile(); @@ -146,6 +147,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: static const std::filesystem::path& _settingsPath(); static const std::filesystem::path& _releaseSettingsPath(); + static winrt::hstring _calculateHash(std::string_view settings, const FILETIME& lastWriteTime); winrt::com_ptr _createNewProfile(const std::wstring_view& name) const; Model::Profile _getProfileForCommandLine(const winrt::hstring& commandLine) const; @@ -162,6 +164,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _validateThemeExists(); // user settings + winrt::hstring _hash; winrt::com_ptr _globals = winrt::make_self(); winrt::com_ptr _baseLayerProfile = winrt::make_self(); winrt::Windows::Foundation::Collections::IObservableVector _allProfiles = winrt::single_threaded_observable_vector(); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl index ff9e46ff1b1..fb69e58ffc8 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -20,12 +20,14 @@ namespace Microsoft.Terminal.Settings.Model static String ApplicationVersion { get; }; static void ExportFile(String path, String content); - + CascadiaSettings(String userJSON, String inboxJSON); CascadiaSettings Copy(); void WriteSettingsToDisk(); + String Hash { get; }; + GlobalAppSettings GlobalSettings { get; }; Profile ProfileDefaults { get; }; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 1d6f71c6344..5fd635e2465 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -793,7 +793,8 @@ void SettingsLoader::_executeGenerator(const IDynamicProfileGenerator& generator Model::CascadiaSettings CascadiaSettings::LoadAll() try { - auto settingsString = ReadUTF8FileIfExists(_settingsPath()).value_or(std::string{}); + FILETIME lastWriteTime{}; + auto settingsString = ReadUTF8FileIfExists(_settingsPath(), false, &lastWriteTime).value_or(std::string{}); auto firstTimeSetup = settingsString.empty(); // If it's the firstTimeSetup and a preview build, then try to @@ -874,6 +875,12 @@ try settings->_warnings.Append(SettingsLoadWarnings::FailedToWriteToSettings); } } + else + { + // lastWriteTime is only valid if mustWriteToDisk is false. + // Additionally WriteSettingsToDisk() updates the _hash for us already. + settings->_hash = _calculateHash(settingsString, lastWriteTime); + } return *settings; } @@ -1015,6 +1022,15 @@ const std::filesystem::path& CascadiaSettings::_releaseSettingsPath() return path; } +// Returns a has (approximately) uniquely identifying the settings.json contents on disk. +winrt::hstring CascadiaSettings::_calculateHash(std::string_view settings, const FILETIME& lastWriteTime) +{ + const auto fileHash = til::hash(settings); + const ULARGE_INTEGER fileTime{ lastWriteTime.dwLowDateTime, lastWriteTime.dwHighDateTime }; + const auto hash = fmt::format(L"{:016x}-{:016x}", fileHash, fileTime.QuadPart); + return winrt::hstring{ hash }; +} + // function Description: // - Returns the full path to the settings file, either within the application // package, or in its unpackaged location. This path is under the "Local @@ -1058,7 +1074,7 @@ winrt::hstring CascadiaSettings::DefaultSettingsPath() // - // Return Value: // - -void CascadiaSettings::WriteSettingsToDisk() const +void CascadiaSettings::WriteSettingsToDisk() { const auto settingsPath = _settingsPath(); @@ -1067,8 +1083,11 @@ void CascadiaSettings::WriteSettingsToDisk() const wbuilder.settings_["indentation"] = " "; wbuilder.settings_["enableYAMLCompatibility"] = true; // suppress spaces around colons + FILETIME lastWriteTime{}; const auto styledString{ Json::writeString(wbuilder, ToJson()) }; - WriteUTF8FileAtomic(settingsPath, styledString); + WriteUTF8FileAtomic(settingsPath, styledString, &lastWriteTime); + + _hash = _calculateHash(styledString, lastWriteTime); // Persists the default terminal choice // GH#10003 - Only do this if _currentDefaultTerminal was actually initialized. diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 8fc28be6e65..bc9100afab8 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -107,7 +107,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model } // 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, const bool elevatedOnly) + std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly, FILETIME* lastWriteTime) { // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) @@ -179,6 +179,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model buffer.erase(0, Utf8Bom.size()); } + if (lastWriteTime) + { + THROW_IF_WIN32_BOOL_FALSE(GetFileTime(file.get(), nullptr, nullptr, lastWriteTime)); + } + return buffer; } @@ -186,11 +191,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model } // Same as ReadUTF8File, but returns an empty optional, if the file couldn't be opened. - std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly) + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly, FILETIME* lastWriteTime) { try { - return { ReadUTF8File(path, elevatedOnly) }; + return { ReadUTF8File(path, elevatedOnly, lastWriteTime) }; } catch (const wil::ResultException& exception) { @@ -203,9 +208,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model } } - void WriteUTF8File(const std::filesystem::path& path, - const std::string_view& content, - const bool elevatedOnly) + void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content, const bool elevatedOnly, FILETIME* lastWriteTime) { SECURITY_ATTRIBUTES sa; // stash the security descriptor here, so it will stay in context until @@ -277,10 +280,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model { THROW_WIN32_MSG(ERROR_WRITE_FAULT, "failed to write whole file"); } + + if (lastWriteTime) + { + THROW_IF_WIN32_BOOL_FALSE(GetFileTime(file.get(), nullptr, nullptr, lastWriteTime)); + } } - void WriteUTF8FileAtomic(const std::filesystem::path& path, - const std::string_view& content) + void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view& content, FILETIME* lastWriteTime) { // GH#10787: rename() will replace symbolic links themselves and not the path they point at. // It's thus important that we first resolve them before generating temporary path. @@ -298,7 +305,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model // * resolve the link manually, which might be less accurate and more prone to race conditions // * write to the file directly, which lets the system resolve the symbolic link but leaves the write non-atomic // The latter is chosen, as this is an edge case and our 'atomic' writes are only best-effort. - WriteUTF8File(path, content); + WriteUTF8File(path, content, false, lastWriteTime); return; } @@ -306,7 +313,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model tmpPath += L".tmp"; // Writing to a file isn't atomic, but... - WriteUTF8File(tmpPath, content); + WriteUTF8File(tmpPath, content, false, lastWriteTime); // renaming one is (supposed to be) atomic. // Wait... "supposed to be"!? Well it's technically not always atomic, diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.h b/src/cascadia/TerminalSettingsModel/FileUtils.h index f898c6ba5bb..f51ad2db9c3 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.h +++ b/src/cascadia/TerminalSettingsModel/FileUtils.h @@ -5,8 +5,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model { std::filesystem::path GetBaseSettingsPath(); std::filesystem::path GetReleaseSettingsPath(); - std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly = false); - std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly = false); - void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content, const bool elevatedOnly = false); - void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view& content); + std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr); + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr); + void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr); + void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view& content, FILETIME* lastWriteTime = nullptr); }