Skip to content

Commit

Permalink
Reduce the number of GUID round trips when looking up profiles.
Browse files Browse the repository at this point in the history 
This commit also moves to storing a reference to the active profile
inside Pane and propagating it out of Pane through Tab. This lets us
duplicate a pane even if its profile is missing, and gives us the
freedom in the future to return a "base" profile (;))

Related to #5047.
  • Loading branch information
@DHowett
DHowett committed Aug 18, 2021
1 parent 638c6d0 commit 9e5fd96
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 115 deletions.
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,11 +753,10 @@ namespace winrt::TerminalApp::implementation
newTerminalArgs = NewTerminalArgs();
}

const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto settings{ TerminalSettings::CreateWithNewTerminalArgs(_settings, newTerminalArgs, *_bindings) };
const auto profile{ _settings.GetProfileForArgs(newTerminalArgs) };

// Manually fill in the evaluated profile.
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profileGuid));
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profile.Guid()));
_OpenNewWindow(false, newTerminalArgs);
actionArgs.Handled(true);
}
Expand Down
39 changes: 20 additions & 19 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Wi
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_focusedBorderBrush = { nullptr };
winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_unfocusedBorderBrush = { nullptr };

Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocused) :
Pane::Pane(const Profile& profile, const TermControl& control, const bool lastFocused) :
_control{ control },
_lastActive{ lastFocused },
_profile{ profile }
Expand Down Expand Up @@ -758,11 +758,9 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
return;
}

const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
if (_profile)
{
auto mode = paneProfile.CloseOnExit();
auto mode = _profile.CloseOnExit();
if ((mode == CloseOnExitMode::Always) ||
(mode == CloseOnExitMode::Graceful && newConnectionState == ConnectionState::Closed))
{
Expand All @@ -786,27 +784,25 @@ void Pane::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspect
{
return;
}
const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
if (_profile)
{
// We don't want to do anything if nothing is set, so check for that first
if (static_cast<int>(paneProfile.BellStyle()) != 0)
if (static_cast<int>(_profile.BellStyle()) != 0)
{
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
{
// Audible is set, play the sound
const auto soundAlias = reinterpret_cast<LPCTSTR>(SND_ALIAS_SYSTEMHAND);
PlaySound(soundAlias, NULL, SND_ALIAS_ID | SND_ASYNC | SND_SENTRY);
}

if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
if (WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
{
_control.BellLightOn();
}

// raise the event with the bool value corresponding to the taskbar flag
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(_profile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
}
}
}
Expand Down Expand Up @@ -955,10 +951,10 @@ void Pane::SetActive()
// Return Value:
// - nullopt if no children of this pane were the last control to be
// focused, else the GUID of the profile of the last control to be focused
std::optional<GUID> Pane::GetFocusedProfile()
Profile Pane::GetFocusedProfile()
{
auto lastFocused = GetActivePane();
return lastFocused ? lastFocused->_profile : std::nullopt;
return lastFocused ? lastFocused->_profile : nullptr;
}

// Method Description:
Expand Down Expand Up @@ -1062,7 +1058,7 @@ void Pane::_FocusFirstChild()
// - profile: The GUID of the profile these settings should apply to.
// Return Value:
// - <none>
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GUID& profile)
void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Profile& profile)
{
if (!_IsLeaf())
{
Expand All @@ -1071,8 +1067,13 @@ void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GU
}
else
{
if (profile == _profile)
// 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
Expand Down Expand Up @@ -1885,7 +1886,7 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
// - The two newly created Panes
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState splitType,
const float splitSize,
const GUID& profile,
const Profile& profile,
const TermControl& control)
{
if (!_IsLeaf())
Expand Down Expand Up @@ -2015,9 +2016,9 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
// Create two new Panes
// Move our control, guid into the first one.
// Move the new guid, control into the second.
_firstChild = std::make_shared<Pane>(_profile.value(), _control);
_firstChild = std::make_shared<Pane>(_profile, _control);
_firstChild->_connectionState = std::exchange(_connectionState, ConnectionState::NotConnected);
_profile = std::nullopt;
_profile = nullptr;
_control = { nullptr };
_secondChild = newPane;

Expand Down
10 changes: 5 additions & 5 deletions src/cascadia/TerminalApp/Pane.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ DEFINE_ENUM_FLAG_OPERATORS(Borders);
class Pane : public std::enable_shared_from_this<Pane>
{
public:
Pane(const GUID& profile,
Pane(const winrt::Microsoft::Terminal::Settings::Model::Profile& profile,
const winrt::Microsoft::Terminal::Control::TermControl& control,
const bool lastFocused = false);

std::shared_ptr<Pane> GetActivePane();
winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl();
std::optional<GUID> GetFocusedProfile();
winrt::Microsoft::Terminal::Settings::Model::Profile GetFocusedProfile();

winrt::Windows::UI::Xaml::Controls::Grid GetRootElement();

Expand All @@ -63,7 +63,7 @@ class Pane : public std::enable_shared_from_this<Pane>
void SetActive();

void UpdateSettings(const winrt::Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings,
const GUID& profile);
const winrt::Microsoft::Terminal::Settings::Model::Profile& profile);
void ResizeContent(const winrt::Windows::Foundation::Size& newSize);
void Relayout();
bool ResizePane(const winrt::Microsoft::Terminal::Settings::Model::ResizeDirection& direction);
Expand All @@ -75,7 +75,7 @@ class Pane : public std::enable_shared_from_this<Pane>

std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Split(winrt::Microsoft::Terminal::Settings::Model::SplitState splitType,
const float splitSize,
const GUID& profile,
const winrt::Microsoft::Terminal::Settings::Model::Profile& profile,
const winrt::Microsoft::Terminal::Control::TermControl& control);
bool ToggleSplitOrientation();
float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const;
Expand Down Expand Up @@ -160,7 +160,7 @@ class Pane : public std::enable_shared_from_this<Pane>
std::optional<uint32_t> _id;

bool _lastActive{ false };
std::optional<GUID> _profile{ std::nullopt };
winrt::Microsoft::Terminal::Settings::Model::Profile _profile{ nullptr };
winrt::event_token _connectionStateChangedToken{ 0 };
winrt::event_token _firstClosedToken{ 0 };
winrt::event_token _secondClosedToken{ 0 };
Expand Down
62 changes: 22 additions & 40 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ namespace winrt::TerminalApp::implementation
HRESULT TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection)
try
{
const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto profile{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto settings{ TerminalSettings::CreateWithNewTerminalArgs(_settings, newTerminalArgs, *_bindings) };

_CreateNewTabFromSettings(profileGuid, settings, existingConnection);
_CreateNewTabWithProfileAndSettings(profile, settings, existingConnection);

const uint32_t tabCount = _tabs.Size();
const bool usedManualProfile = (newTerminalArgs != nullptr) &&
(newTerminalArgs.ProfileIndex() != nullptr ||
newTerminalArgs.Profile().empty());

// Lookup the name of the color scheme used by this profile.
const auto scheme = _settings.GetColorSchemeForProfile(profileGuid);
const auto scheme = _settings.GetColorSchemeForProfile(profile);
// If they explicitly specified `null` as the scheme (indicating _no_ scheme), log
// that as the empty string.
const auto schemeName = scheme ? scheme.Name() : L"\0";
Expand All @@ -82,7 +82,7 @@ namespace winrt::TerminalApp::implementation
TraceLoggingUInt32(1u, "EventVer", "Version of this event"),
TraceLoggingUInt32(tabCount, "TabCount", "Count of tabs currently opened in TerminalApp"),
TraceLoggingBool(usedManualProfile, "ProfileSpecified", "Whether the new tab specified a profile explicitly"),
TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"),
TraceLoggingGuid(profile.Guid(), "ProfileGuid", "The GUID of the profile spawned in the new tab"),
TraceLoggingBool(settings.DefaultSettings().UseAcrylic(), "UseAcrylic", "The acrylic preference from the settings"),
TraceLoggingFloat64(settings.DefaultSettings().TintOpacity(), "TintOpacity", "Opacity preference from the settings"),
TraceLoggingWideString(settings.DefaultSettings().FontFace().c_str(), "FontFace", "Font face chosen in the settings"),
Expand Down Expand Up @@ -175,10 +175,9 @@ namespace winrt::TerminalApp::implementation
_tabView.TabItems().Append(tabViewItem);

// Set this tab's icon to the icon from the user's profile
if (const auto profileGuid = newTabImpl->GetFocusedProfile())
if (const auto profile{ newTabImpl->GetFocusedProfile() })
{
const auto profile = _settings.FindProfile(profileGuid.value());
if (profile != nullptr && !profile.Icon().empty())
if (!profile.Icon().empty())
{
newTabImpl->UpdateIcon(profile.Icon());
}
Expand Down Expand Up @@ -233,14 +232,14 @@ namespace winrt::TerminalApp::implementation
// - Creates a new tab with the given settings. If the tab bar is not being
// currently displayed, it will be shown.
// Arguments:
// - profileGuid: ID to use to lookup profile settings for this connection
// - profile: profile settings for this connection
// - settings: the TerminalSettings object to use to create the TerminalControl with.
// - existingConnection: optionally receives a connection from the outside world instead of attempting to create one
void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, const TerminalSettingsCreateResult& settings, TerminalConnection::ITerminalConnection existingConnection)
void TerminalPage::_CreateNewTabWithProfileAndSettings(const Profile& profile, const TerminalSettingsCreateResult& settings, TerminalConnection::ITerminalConnection existingConnection)
{
// Initialize the new tab
// Create a connection based on the values in our settings object if we weren't given one.
auto connection = existingConnection ? existingConnection : _CreateConnectionFromSettings(profileGuid, settings.DefaultSettings());
auto connection = existingConnection ? existingConnection : _CreateConnectionFromSettings(profile, settings.DefaultSettings());

// If we had an `existingConnection`, then this is an inbound handoff from somewhere else.
// We need to tell it about our size information so it can match the dimensions of what
Expand Down Expand Up @@ -268,7 +267,7 @@ namespace winrt::TerminalApp::implementation
// This way, when we do a settings reload we just update the parent and the overrides remain
auto term = _InitControl(settings, connection);

auto newTabImpl = winrt::make_self<TerminalTab>(profileGuid, term);
auto newTabImpl = winrt::make_self<TerminalTab>(profile, term);
_RegisterTerminalEvents(term);
_InitializeTab(newTabImpl);

Expand All @@ -277,7 +276,7 @@ namespace winrt::TerminalApp::implementation
auto newControl = _InitControl(settings, debugConnection);
_RegisterTerminalEvents(newControl);
// Split (auto) with the debug tap.
newTabImpl->SplitPane(SplitState::Automatic, 0.5f, profileGuid, newControl);
newTabImpl->SplitPane(SplitState::Automatic, 0.5f, profile, newControl);
}
}

Expand All @@ -288,19 +287,9 @@ namespace winrt::TerminalApp::implementation
// - tab: the Tab to update the title for.
void TerminalPage::_UpdateTabIcon(TerminalTab& tab)
{
const auto lastFocusedProfileOpt = tab.GetFocusedProfile();
if (lastFocusedProfileOpt.has_value())
if (const auto profile = tab.GetFocusedProfile())
{
const auto lastFocusedProfile = lastFocusedProfileOpt.value();
const auto matchingProfile = _settings.FindProfile(lastFocusedProfile);
if (matchingProfile)
{
tab.UpdateIcon(matchingProfile.Icon());
}
else
{
tab.UpdateIcon({});
}
tab.UpdateIcon(profile.Icon());
}
}

Expand Down Expand Up @@ -354,31 +343,24 @@ namespace winrt::TerminalApp::implementation
{
try
{
// TODO: GH#5047 - In the future, we should get the Profile of
// the focused pane, and use that to build a new instance of the
// settings so we can duplicate this tab/pane.
//
// Currently, if the profile doesn't exist anymore in our
// settings, we'll silently do nothing.
// TODO: GH#5047 - We're duplicating the whole profile, which might
// be a dangling reference to old settings.
//
// In the future, it will be preferable to just duplicate the
// current control's settings, but we can't do that currently,
// because we won't be able to create a new instance of the
// connection without keeping an instance of the original Profile
// object around.

const auto& profileGuid = tab.GetFocusedProfile();
if (profileGuid.has_value())
// In the future, it may be preferable to just duplicate the
// current control's live settings (which will include changes
// made through VT).

if (const auto profile = tab.GetFocusedProfile())
{
const auto settingsCreateResult{ TerminalSettings::CreateWithProfileByID(_settings, profileGuid.value(), *_bindings) };
const auto settingsCreateResult{ TerminalSettings::CreateWithProfile(_settings, profile, *_bindings) };
const auto workingDirectory = tab.GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
{
settingsCreateResult.DefaultSettings().StartingDirectory(workingDirectory);
}

_CreateNewTabFromSettings(profileGuid.value(), settingsCreateResult);
_CreateNewTabWithProfileAndSettings(profile, settingsCreateResult);

const auto runtimeTabText{ tab.GetTabText() };
if (!runtimeTabText.empty())
Expand Down
Loading

0 comments on commit 9e5fd96

Please sign in to comment.