Only iterate panes one time when updating settings (#10997)

The original code for settings reload iterated the entire tree of panes
for every profile in the new settings (O(mn)) 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.
This commit is contained in:
Dustin L. Howett 2021-08-23 12:20:08 -07:00 committed by GitHub
parent f6f5598c9c
commit 10992b77a0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 77 additions and 66 deletions

View file

@ -1049,10 +1049,7 @@ void Pane::_FocusFirstChild()
} }
// Method Description: // Method Description:
// - Attempts to update the settings of this pane or any children of this pane. // - Updates the settings of this pane, presuming that it is a leaf.
// * 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.
// Arguments: // Arguments:
// - settings: The new TerminalSettings to apply to any matching controls // - settings: The new TerminalSettings to apply to any matching controls
// - profile: The profile from which these settings originated. // - profile: The profile from which these settings originated.
@ -1060,37 +1057,24 @@ void Pane::_FocusFirstChild()
// - <none> // - <none>
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile) void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile)
{ {
if (!_IsLeaf()) assert(_IsLeaf());
_profile = profile;
auto controlSettings = _control.Settings().as<TerminalSettings>();
// 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)
{ {
_firstChild->UpdateSettings(settings, profile); // Note: the unfocused settings needs to be entirely unchanged _except_ we need to
_secondChild->UpdateSettings(settings, profile); // 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
else // sure the unfocused settings inherit from that.
{ unfocusedSettings.SetParent(controlSettings);
// 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<TerminalSettings>();
// 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();
}
} }
_control.UnfocusedAppearance(unfocusedSettings);
_control.UpdateSettings();
} }
// Method Description: // Method Description:

View file

@ -55,6 +55,14 @@ public:
winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl(); winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl();
winrt::Microsoft::Terminal::Settings::Model::Profile GetFocusedProfile(); winrt::Microsoft::Terminal::Settings::Model::Profile GetFocusedProfile();
// 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(); winrt::Windows::UI::Xaml::Controls::Grid GetRootElement();
bool WasLastFocused() const noexcept; bool WasLastFocused() const noexcept;

View file

@ -1996,36 +1996,56 @@ namespace winrt::TerminalApp::implementation
_HookupKeyBindings(_settings.ActionMap()); _HookupKeyBindings(_settings.ActionMap());
// Refresh UI elements // Refresh UI elements
auto profiles = _settings.ActiveProfiles();
for (const auto& profile : profiles)
{
try
{
auto settings{ TerminalSettings::CreateWithProfile(_settings, profile, *_bindings) };
for (auto tab : _tabs) // 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.
if (auto terminalTab = _GetTerminalTabImpl(tab)) std::unordered_map<winrt::guid, std::pair<Profile, TerminalSettingsCreateResult>> profileGuidSettingsMap;
{ const auto profileDefaults{ _settings.ProfileDefaults() };
terminalTab->UpdateSettings(settings, profile); const auto allProfiles{ _settings.AllProfiles() };
}
} profileGuidSettingsMap.reserve(allProfiles.Size() + 1);
}
CATCH_LOG(); // Include the Defaults profile for consideration
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 });
} }
// GH#2455: If there are any panes with controls that had been for (const auto& tab : _tabs)
// 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)) if (auto terminalTab{ _GetTerminalTabImpl(tab) })
{ {
terminalTab->UpdateSettings();
// 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() })
{
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);
}
}
return false;
});
// 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); _UpdateTabIcon(*terminalTab);
// Force the TerminalTab to re-grab its currently active control's title. // Force the TerminalTab to re-grab its currently active control's title.

View file

@ -263,16 +263,13 @@ namespace winrt::TerminalApp::implementation
} }
// Method Description: // Method Description:
// - Attempts to update the settings of this tab's tree of panes. // - Attempts to update the settings that apply to this tab.
// Arguments: // - Panes are handled elsewhere, by somebody who can establish broader knowledge
// - settings: The new TerminalSettingsCreateResult to apply to any matching controls // of the settings that apply to all tabs.
// - profile: The GUID of the profile these settings should apply to.
// Return Value: // Return Value:
// - <none> // - <none>
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 // The tabWidthMode may have changed, update the header control accordingly
_UpdateHeaderControlMaxWidth(); _UpdateHeaderControlMaxWidth();
} }

View file

@ -62,7 +62,7 @@ namespace winrt::TerminalApp::implementation
bool SwapPane(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction); bool SwapPane(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction);
bool FocusPane(const uint32_t id); 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(); winrt::fire_and_forget UpdateTitle();
void Shutdown() override; void Shutdown() override;
@ -90,6 +90,8 @@ namespace winrt::TerminalApp::implementation
std::shared_ptr<Pane> GetActivePane() const; std::shared_ptr<Pane> GetActivePane() const;
winrt::TerminalApp::TaskbarState GetCombinedTaskbarState() const; winrt::TerminalApp::TaskbarState GetCombinedTaskbarState() const;
std::shared_ptr<Pane> GetRootPane() const { return _rootPane; }
winrt::TerminalApp::TerminalTabStatus TabStatus() winrt::TerminalApp::TerminalTabStatus TabStatus()
{ {
return _tabStatus; return _tabStatus;