From fe9c4588b77e08d282510d6c9bd13085e810ba17 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Thu, 19 Aug 2021 13:27:31 -0700 Subject: [PATCH 1/4] Only iterate panes one time when updating settings The original code for settings reload iterated the entire tree of panes for every profile in the new settings, and constructed a TerminalSettings object for every profile even if it later went unused. This implementation: 1. Collects all new profiles keyed by guid 1.a. Adds the "defaults" profile to the map 2. Iterates every pane, just once, and updates its profile if it shows up in the list by GUID. I've merged all of the per-tab code into a single loop. Because of 1.a., this code can now update panes that are hosting the "base" profile. --- src/cascadia/TerminalApp/Pane.cpp | 49 +++++++---------- src/cascadia/TerminalApp/Pane.h | 1 + src/cascadia/TerminalApp/TerminalPage.cpp | 65 ++++++++++++++--------- src/cascadia/TerminalApp/TerminalTab.cpp | 4 +- src/cascadia/TerminalApp/TerminalTab.h | 4 +- 5 files changed, 64 insertions(+), 59 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 127884b587c..a6466d30e13 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1060,37 +1060,24 @@ void Pane::_FocusFirstChild() // - void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile) { - if (!_IsLeaf()) - { - _firstChild->UpdateSettings(settings, profile); - _secondChild->UpdateSettings(settings, profile); - } - else - { - // Because this call may be coming in with a different settings tree, - // we want to map the incoming profile based on its GUID. - // Failure to find a matching profile will result in a pane holding - // a reference to a deleted profile (which is okay!). - if (profile.Guid() == _profile.Guid()) - { - _profile = profile; - auto controlSettings = _control.Settings().as(); - // Update the parent of the control's settings object (and not the object itself) so - // that any overrides made by the control don't get affected by the reload - controlSettings.SetParent(settings.DefaultSettings()); - auto unfocusedSettings{ settings.UnfocusedSettings() }; - if (unfocusedSettings) - { - // Note: the unfocused settings needs to be entirely unchanged _except_ we need to - // set its parent to the settings object that lives in the control. This is because - // the overrides made by the control live in that settings object, so we want to make - // sure the unfocused settings inherit from that. - unfocusedSettings.SetParent(controlSettings); - } - _control.UnfocusedAppearance(unfocusedSettings); - _control.UpdateSettings(); - } - } + assert(_IsLeaf()); + + _profile = profile; + auto controlSettings = _control.Settings().as(); + // Update the parent of the control's settings object (and not the object itself) so + // that any overrides made by the control don't get affected by the reload + controlSettings.SetParent(settings.DefaultSettings()); + auto unfocusedSettings{ settings.UnfocusedSettings() }; + if (unfocusedSettings) + { + // Note: the unfocused settings needs to be entirely unchanged _except_ we need to + // set its parent to the settings object that lives in the control. This is because + // the overrides made by the control live in that settings object, so we want to make + // sure the unfocused settings inherit from that. + unfocusedSettings.SetParent(controlSettings); + } + _control.UnfocusedAppearance(unfocusedSettings); + _control.UpdateSettings(); } // Method Description: diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 681fd7ba93e..2216bc16568 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -54,6 +54,7 @@ class Pane : public std::enable_shared_from_this std::shared_ptr GetActivePane(); winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl(); winrt::Microsoft::Terminal::Settings::Model::Profile GetFocusedProfile(); + winrt::Microsoft::Terminal::Settings::Model::Profile GetProfile() const { return _profile; } winrt::Windows::UI::Xaml::Controls::Grid GetRootElement(); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 07bd2e19f47..d2b6828a3d3 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1996,36 +1996,53 @@ namespace winrt::TerminalApp::implementation _HookupKeyBindings(_settings.ActionMap()); // Refresh UI elements - auto profiles = _settings.ActiveProfiles(); - for (const auto& profile : profiles) + + // Mapping by GUID isn't _excellent_ because the defaults profile doesn't have a stable GUID; however, + // when we stabilize its guid this will become fully safe. + std::unordered_map> _profileGuidSettingsMap; + + // Include the Defaults profile for consideration + const auto profileDefaults{ _settings.ProfileDefaults() }; + _profileGuidSettingsMap.insert_or_assign(profileDefaults.Guid(), std::pair{ profileDefaults, nullptr }); + for (const auto& newProfile : _settings.AllProfiles()) { - try + // Avoid creating a TerminalSettings right now. They're not totally cheap, and we suspect that users with many + // panes may not be using all of their profiles at the same time. Lazy evaluation is king! + _profileGuidSettingsMap.insert_or_assign(newProfile.Guid(), std::pair{ newProfile, nullptr }); + } + + for (const auto& tab : _tabs) + { + if (auto terminalTab{ _GetTerminalTabImpl(tab) }) { - auto settings{ TerminalSettings::CreateWithProfile(_settings, profile, *_bindings) }; + terminalTab->UpdateSettings(); - for (auto tab : _tabs) - { - if (auto terminalTab = _GetTerminalTabImpl(tab)) + // Manually enumerate the panes in each tab; this will let us recycle TerminalSettings + // objects but only have to iterate one time. + terminalTab->GetRootPane()->WalkTree([&](auto&& pane) { + if (const auto profile{ pane->GetProfile() }) { - terminalTab->UpdateSettings(settings, profile); + const auto found{ _profileGuidSettingsMap.find(profile.Guid()) }; + // GH#2455: If there are any panes with controls that had been + // initialized with a Profile that no longer exists in our list of + // profiles, we'll leave it unmodified. The profile doesn't exist + // anymore, so we can't possibly update its settings. + if (found != _profileGuidSettingsMap.cend()) + { + auto& pair{ found->second }; + if (!pair.second) + { + pair.second = TerminalSettings::CreateWithProfile(_settings, pair.first, *_bindings); + } + pane->UpdateSettings(pair.second, pair.first); + } } - } - } - CATCH_LOG(); - } + return false; + }); - // GH#2455: If there are any panes with controls that had been - // initialized with a Profile that no longer exists in our list of - // profiles, we'll leave it unmodified. The profile doesn't exist - // anymore, so we can't possibly update its settings. - - // Update the icon of the tab for the currently focused profile in that tab. - // Only do this for TerminalTabs. Other types of tabs won't have multiple panes - // and profiles so the Title and Icon will be set once and only once on init. - for (auto tab : _tabs) - { - if (auto terminalTab = _GetTerminalTabImpl(tab)) - { + // Update the icon of the tab for the currently focused profile in that tab. + // Only do this for TerminalTabs. Other types of tabs won't have multiple panes + // and profiles so the Title and Icon will be set once and only once on init. _UpdateTabIcon(*terminalTab); // Force the TerminalTab to re-grab its currently active control's title. diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index c3fc2324219..d67e4cd70e8 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -269,10 +269,8 @@ namespace winrt::TerminalApp::implementation // - profile: The GUID of the profile these settings should apply to. // Return Value: // - - void TerminalTab::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile) + void TerminalTab::UpdateSettings() { - _rootPane->UpdateSettings(settings, profile); - // The tabWidthMode may have changed, update the header control accordingly _UpdateHeaderControlMaxWidth(); } diff --git a/src/cascadia/TerminalApp/TerminalTab.h b/src/cascadia/TerminalApp/TerminalTab.h index b72372d18c2..4952fad4518 100644 --- a/src/cascadia/TerminalApp/TerminalTab.h +++ b/src/cascadia/TerminalApp/TerminalTab.h @@ -62,7 +62,7 @@ namespace winrt::TerminalApp::implementation bool SwapPane(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction); bool FocusPane(const uint32_t id); - void UpdateSettings(const Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings, const winrt::Microsoft::Terminal::Settings::Model::Profile& profile); + void UpdateSettings(); winrt::fire_and_forget UpdateTitle(); void Shutdown() override; @@ -90,6 +90,8 @@ namespace winrt::TerminalApp::implementation std::shared_ptr GetActivePane() const; winrt::TerminalApp::TaskbarState GetCombinedTaskbarState() const; + std::shared_ptr GetRootPane() const { return _rootPane; } + winrt::TerminalApp::TerminalTabStatus TabStatus() { return _tabStatus; From dc9cc1913b4bfb9b76006e0d2933da59453225f7 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Fri, 20 Aug 2021 15:20:30 -0700 Subject: [PATCH 2/4] PR feedback --- src/cascadia/TerminalApp/Pane.h | 9 ++++++++- src/cascadia/TerminalApp/TerminalTab.cpp | 7 +++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 2216bc16568..76bab850b88 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -54,7 +54,14 @@ class Pane : public std::enable_shared_from_this std::shared_ptr GetActivePane(); winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl(); winrt::Microsoft::Terminal::Settings::Model::Profile GetFocusedProfile(); - winrt::Microsoft::Terminal::Settings::Model::Profile GetProfile() const { return _profile; } + + // Method Description: + // - If this is a leaf pane, return its profile. + // - If this is a branch/root pane, return nullptr. + winrt::Microsoft::Terminal::Settings::Model::Profile GetProfile() const + { + return _profile; + } winrt::Windows::UI::Xaml::Controls::Grid GetRootElement(); diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index d67e4cd70e8..6f415754c38 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -263,10 +263,9 @@ namespace winrt::TerminalApp::implementation } // Method Description: - // - Attempts to update the settings of this tab's tree of panes. - // Arguments: - // - settings: The new TerminalSettingsCreateResult to apply to any matching controls - // - profile: The GUID of the profile these settings should apply to. + // - Attempts to update the settings that apply to this tab. + // - Panes are handled elsewhere, by somebody who can establish broader knowledge + // of the settings that apply to all tabs. // Return Value: // - void TerminalTab::UpdateSettings() From 82aef2f3fd562102328136cdb0463b21abc65f4e Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Mon, 23 Aug 2021 10:09:15 -0700 Subject: [PATCH 3/4] PR feedback --- src/cascadia/TerminalApp/Pane.cpp | 5 +---- src/cascadia/TerminalApp/TerminalPage.cpp | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index a6466d30e13..75def5f5830 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1049,10 +1049,7 @@ void Pane::_FocusFirstChild() } // Method Description: -// - Attempts to update the settings of this pane or any children of this pane. -// * If this pane is a leaf, and our profile guid matches the parameter, then -// we'll apply the new settings to our control. -// * If we're not a leaf, we'll recurse on our children. +// - Updates the settings of this pane, presuming that it is a leaf. // Arguments: // - settings: The new TerminalSettings to apply to any matching controls // - profile: The profile from which these settings originated. diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d2b6828a3d3..7bbcf7b9f69 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -2003,12 +2003,12 @@ namespace winrt::TerminalApp::implementation // Include the Defaults profile for consideration const auto profileDefaults{ _settings.ProfileDefaults() }; - _profileGuidSettingsMap.insert_or_assign(profileDefaults.Guid(), std::pair{ profileDefaults, nullptr }); + _profileGuidSettingsMap.insert_or_assign(profileDefaults.Guid(), std::pair{ profileDefaults, nullptr }); for (const auto& newProfile : _settings.AllProfiles()) { // Avoid creating a TerminalSettings right now. They're not totally cheap, and we suspect that users with many // panes may not be using all of their profiles at the same time. Lazy evaluation is king! - _profileGuidSettingsMap.insert_or_assign(newProfile.Guid(), std::pair{ newProfile, nullptr }); + _profileGuidSettingsMap.insert_or_assign(newProfile.Guid(), std::pair{ newProfile, nullptr }); } for (const auto& tab : _tabs) From 3af12d2b45f44e795aa316f73fc2f3bf48a89ec2 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Mon, 23 Aug 2021 13:15:20 -0500 Subject: [PATCH 4/4] PR feedbacc III: feedbacc harder --- src/cascadia/TerminalApp/TerminalPage.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 7bbcf7b9f69..12cec35949f 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1999,16 +1999,19 @@ namespace winrt::TerminalApp::implementation // Mapping by GUID isn't _excellent_ because the defaults profile doesn't have a stable GUID; however, // when we stabilize its guid this will become fully safe. - std::unordered_map> _profileGuidSettingsMap; + std::unordered_map> profileGuidSettingsMap; + const auto profileDefaults{ _settings.ProfileDefaults() }; + const auto allProfiles{ _settings.AllProfiles() }; + + profileGuidSettingsMap.reserve(allProfiles.Size() + 1); // Include the Defaults profile for consideration - const auto profileDefaults{ _settings.ProfileDefaults() }; - _profileGuidSettingsMap.insert_or_assign(profileDefaults.Guid(), std::pair{ profileDefaults, nullptr }); - for (const auto& newProfile : _settings.AllProfiles()) + profileGuidSettingsMap.insert_or_assign(profileDefaults.Guid(), std::pair{ profileDefaults, nullptr }); + for (const auto& newProfile : allProfiles) { // Avoid creating a TerminalSettings right now. They're not totally cheap, and we suspect that users with many // panes may not be using all of their profiles at the same time. Lazy evaluation is king! - _profileGuidSettingsMap.insert_or_assign(newProfile.Guid(), std::pair{ newProfile, nullptr }); + profileGuidSettingsMap.insert_or_assign(newProfile.Guid(), std::pair{ newProfile, nullptr }); } for (const auto& tab : _tabs) @@ -2022,12 +2025,12 @@ namespace winrt::TerminalApp::implementation terminalTab->GetRootPane()->WalkTree([&](auto&& pane) { if (const auto profile{ pane->GetProfile() }) { - const auto found{ _profileGuidSettingsMap.find(profile.Guid()) }; + const auto found{ profileGuidSettingsMap.find(profile.Guid()) }; // GH#2455: If there are any panes with controls that had been // initialized with a Profile that no longer exists in our list of // profiles, we'll leave it unmodified. The profile doesn't exist // anymore, so we can't possibly update its settings. - if (found != _profileGuidSettingsMap.cend()) + if (found != profileGuidSettingsMap.cend()) { auto& pair{ found->second }; if (!pair.second)