From ba08dd21745a023c07f4269ba0144de9a3d86818 Mon Sep 17 00:00:00 2001 From: PankajBhojwani Date: Fri, 5 Aug 2022 12:13:57 -0700 Subject: [PATCH] Use only one tab color picker for all tabs, delay load it (#13674) ## Summary of the Pull Request - We only ever have 1 color picker now, instead of each tab having its own - `TerminalPage` constructs this color picker (upon first request for it) - `TerminalPage` attaches the color picker to the tab that requested for it - `TerminalTab` detaches the color picker when it is done with it, so that `TerminalPage` can attach it to another tab later on ## References #5907 ## Validation Steps Performed User-end behaviour is the same --- .../TerminalApp/AppActionHandlers.cpp | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 14 ++++ src/cascadia/TerminalApp/TerminalPage.h | 1 + src/cascadia/TerminalApp/TerminalTab.cpp | 64 +++++++++++++------ src/cascadia/TerminalApp/TerminalTab.h | 11 +++- 5 files changed, 70 insertions(+), 22 deletions(-) diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index 10a7f8cfa31..6b2d5afc7ba 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -606,7 +606,7 @@ namespace winrt::TerminalApp::implementation { if (const auto activeTab{ _GetFocusedTabImpl() }) { - activeTab->ActivateColorPicker(); + activeTab->RequestColorPicker(); } args.Handled(true); } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index eb8f02f06b8..32d4fb67299 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1509,6 +1509,20 @@ namespace winrt::TerminalApp::implementation } }); + hostingTab.ColorPickerRequested([weakTab, weakThis]() { + auto page{ weakThis.get() }; + auto tab{ weakTab.get() }; + if (page && tab) + { + if (!page->_tabColorPicker) + { + page->_tabColorPicker = winrt::make(); + } + + tab->AttachColorPicker(page->_tabColorPicker); + } + }); + // Add an event handler for when the terminal or tab wants to set a // progress indicator on the taskbar hostingTab.TaskbarProgressChanged({ get_weak(), &TerminalPage::_SetTaskbarProgressHandler }); diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index c0081b54d39..00fac93ec44 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -174,6 +174,7 @@ namespace winrt::TerminalApp::implementation TerminalApp::TabRowControl _tabRow{ nullptr }; Windows::UI::Xaml::Controls::Grid _tabContent{ nullptr }; Microsoft::UI::Xaml::Controls::SplitButton _newTabButton{ nullptr }; + winrt::TerminalApp::ColorPickupFlyout _tabColorPicker{ nullptr }; Microsoft::Terminal::Settings::Model::CascadiaSettings _settings{ nullptr }; diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index 3b1307c9cab..6ef8c735853 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -648,6 +648,46 @@ namespace winrt::TerminalApp::implementation } } + // Method Description: + // - Attaches the given color picker to ourselves + // - Typically will be called after we have sent a request for the color picker + // Arguments: + // - colorPicker: The color picker that we should attach to ourselves + // Return Value: + // - + void TerminalTab::AttachColorPicker(TerminalApp::ColorPickupFlyout& colorPicker) + { + auto weakThis{ get_weak() }; + + _tabColorPickup = colorPicker; + + _colorSelectedToken = _tabColorPickup.ColorSelected([weakThis](auto newTabColor) { + if (auto tab{ weakThis.get() }) + { + tab->SetRuntimeTabColor(newTabColor); + } + }); + + _colorClearedToken = _tabColorPickup.ColorCleared([weakThis]() { + if (auto tab{ weakThis.get() }) + { + tab->ResetRuntimeTabColor(); + } + }); + + _pickerClosedToken = _tabColorPickup.Closed([weakThis](auto&&, auto&&) { + if (auto tab{ weakThis.get() }) + { + tab->_tabColorPickup.ColorSelected(tab->_colorSelectedToken); + tab->_tabColorPickup.ColorCleared(tab->_colorClearedToken); + tab->_tabColorPickup.Closed(tab->_pickerClosedToken); + tab->_tabColorPickup = nullptr; + } + }); + + _tabColorPickup.ShowAt(TabViewItem()); + } + // Method Description: // - Find the currently active pane, and then switch the split direction of // its parent. E.g. switch from Horizontal to Vertical. @@ -1184,27 +1224,12 @@ namespace winrt::TerminalApp::implementation chooseColorMenuItem.Click([weakThis](auto&&, auto&&) { if (auto tab{ weakThis.get() }) { - tab->ActivateColorPicker(); + tab->RequestColorPicker(); } }); chooseColorMenuItem.Text(RS_(L"TabColorChoose")); chooseColorMenuItem.Icon(colorPickSymbol); - // Color Picker (it's convenient to have it here) - _tabColorPickup.ColorSelected([weakThis](auto newTabColor) { - if (auto tab{ weakThis.get() }) - { - tab->SetRuntimeTabColor(newTabColor); - } - }); - - _tabColorPickup.ColorCleared([weakThis]() { - if (auto tab{ weakThis.get() }) - { - tab->ResetRuntimeTabColor(); - } - }); - Controls::MenuFlyoutItem renameTabMenuItem; { // "Rename Tab" @@ -1597,14 +1622,15 @@ namespace winrt::TerminalApp::implementation } // Method Description: - // - Display the tab color picker at the location of the TabViewItem for this tab. + // - Send an event to request for the color picker + // - The listener should attach the color picker via AttachColorPicker() // Arguments: // - // Return Value: // - - void TerminalTab::ActivateColorPicker() + void TerminalTab::RequestColorPicker() { - _tabColorPickup.ShowAt(TabViewItem()); + _ColorPickerRequestedHandlers(); } // Method Description: diff --git a/src/cascadia/TerminalApp/TerminalTab.h b/src/cascadia/TerminalApp/TerminalTab.h index d5e45ca0a0b..5f7a4c8ed82 100644 --- a/src/cascadia/TerminalApp/TerminalTab.h +++ b/src/cascadia/TerminalApp/TerminalTab.h @@ -37,6 +37,8 @@ namespace winrt::TerminalApp::implementation std::shared_ptr DetachPane(); void AttachPane(std::shared_ptr pane); + void AttachColorPicker(winrt::TerminalApp::ColorPickupFlyout& colorPicker); + void SplitPane(winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType, const float splitSize, std::shared_ptr newPane); @@ -74,7 +76,7 @@ namespace winrt::TerminalApp::implementation void ThemeColor(const winrt::Microsoft::Terminal::Settings::Model::ThemeColor& color); void SetRuntimeTabColor(const winrt::Windows::UI::Color& color); void ResetRuntimeTabColor(); - void ActivateColorPicker(); + void RequestColorPicker(); void UpdateZoom(std::shared_ptr newFocus); void ToggleZoom(); @@ -105,6 +107,7 @@ namespace winrt::TerminalApp::implementation WINRT_CALLBACK(SplitTabRequested, winrt::delegate<>); WINRT_CALLBACK(FindRequested, winrt::delegate<>); WINRT_CALLBACK(ExportTabRequested, winrt::delegate<>); + WINRT_CALLBACK(ColorPickerRequested, winrt::delegate<>); TYPED_EVENT(TaskbarProgressChanged, IInspectable, IInspectable); private: @@ -113,12 +116,16 @@ namespace winrt::TerminalApp::implementation std::shared_ptr _zoomedPane{ nullptr }; winrt::hstring _lastIconPath{}; - winrt::TerminalApp::ColorPickupFlyout _tabColorPickup{}; std::optional _runtimeTabColor{}; winrt::TerminalApp::TabHeaderControl _headerControl{}; winrt::TerminalApp::TerminalTabStatus _tabStatus{}; winrt::Microsoft::Terminal::Settings::Model::ThemeColor _themeColor{ nullptr }; + winrt::TerminalApp::ColorPickupFlyout _tabColorPickup{ nullptr }; + winrt::event_token _colorSelectedToken; + winrt::event_token _colorClearedToken; + winrt::event_token _pickerClosedToken; + struct ControlEventTokens { winrt::event_token titleToken;