Reduce the number of GUID round trips when looking up profiles.

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.

Author: DHowett

src/cascadia/TerminalApp/AppActionHandlers.cpp

@@ -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);
     }

src/cascadia/TerminalApp/Pane.cpp

@@ -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 }
@@ -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))
         {
@@ -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));
         }
     }
 }
@@ -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:
@@ -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())
     {
@@ -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
@@ -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())
@@ -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;
 

src/cascadia/TerminalApp/Pane.h

@@ -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();
 
@@ -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);
@@ -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;
@@ -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 };

src/cascadia/TerminalApp/TabManagement.cpp

@@ -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";
@@ -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"),
@@ -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());
             }
@@ -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
@@ -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);
 
@@ -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);
         }
     }
 
@@ -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());
         }
     }
 
@@ -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())