Incorporate PR feedback microsoft#2994

- remove redundant files - disable the alpha settings in the color picker - move the winrt/Windows.UI.Popups.h to pch.h - move the VisualState switching code to a helper method - use weak ptr in the color pickup flyout - use weak_ptr in lambdas instead of [this]
gbaychev · Jan 6, 2020 · 1cf9407 · 1cf9407
1 parent a5aa717
commit 1cf9407
Show file tree

Hide file tree

Showing 11 changed files with 105 additions and 156 deletions.
diff --git a/src/cascadia/TerminalApp/ColorFlyoutSubItem.cpp b/src/cascadia/TerminalApp/ColorFlyoutSubItem.cpp
diff --git a/src/cascadia/TerminalApp/ColorFlyoutSubItem.h b/src/cascadia/TerminalApp/ColorFlyoutSubItem.h
diff --git a/src/cascadia/TerminalApp/ColorFlyoutSubItem.idl b/src/cascadia/TerminalApp/ColorFlyoutSubItem.idl
diff --git a/src/cascadia/TerminalApp/ColorFlyoutSubItem.xaml b/src/cascadia/TerminalApp/ColorFlyoutSubItem.xaml
diff --git a/src/cascadia/TerminalApp/ColorPickupFlyout.cpp b/src/cascadia/TerminalApp/ColorPickupFlyout.cpp
@@ -26,9 +26,7 @@ namespace winrt::TerminalApp::implementation
         btnCustomColor().Content(winrt::box_value(customColorText));
         btnClearColor().Content(winrt::box_value(clearColorText));
 
-        customColorPicker().ColorChanged([this](const auto&, const Windows::UI::Xaml::Controls::ColorChangedEventArgs& args) {
-            _colorSelected(args.NewColor());
-        });
+        customColorPicker().ColorChanged({ get_weak(), &ColorPickupFlyout::_ColorChangedHandler });
     }
 
     // Method Description:
@@ -104,6 +102,11 @@ namespace winrt::TerminalApp::implementation
         Hide();
     }
 
+    void ColorPickupFlyout::_ColorChangedHandler(const Windows::UI::Xaml::Controls::ColorPicker&, const Windows::UI::Xaml::Controls::ColorChangedEventArgs& args)
+    {
+        _colorSelected(args.NewColor());
+    }
+
     DEFINE_EVENT(ColorPickupFlyout, ColorSelected, _colorSelected, TerminalApp::ColorSelectedArgs);
     DEFINE_EVENT(ColorPickupFlyout, ColorCleared, _colorCleared, TerminalApp::ColorClearedArgs);
 }
diff --git a/src/cascadia/TerminalApp/ColorPickupFlyout.h b/src/cascadia/TerminalApp/ColorPickupFlyout.h
@@ -15,6 +15,9 @@ namespace winrt::TerminalApp::implementation
 
         DECLARE_EVENT(ColorSelected, _colorSelected, TerminalApp::ColorSelectedArgs);
         DECLARE_EVENT(ColorCleared, _colorCleared, TerminalApp::ColorClearedArgs);
+
+    private:
+        void _ColorChangedHandler(const Windows::UI::Xaml::Controls::ColorPicker&, const Windows::UI::Xaml::Controls::ColorChangedEventArgs& args);
     };
 }
 

diff --git a/src/cascadia/TerminalApp/ColorPickupFlyout.xaml b/src/cascadia/TerminalApp/ColorPickupFlyout.xaml
@@ -146,8 +146,8 @@
                      IsColorChannelTextInputVisible="True"
                      IsHexInputVisible="True"
                      IsAlphaEnabled="False"
-                     IsAlphaSliderVisible="True"
-                     IsAlphaTextInputVisible="True"
+                     IsAlphaSliderVisible="False"
+                     IsAlphaTextInputVisible="False"
                      FontSize="10"
                      Grid.Row="0"
                      >

diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp
@@ -6,7 +6,6 @@
 #include "ColorPickupFlyout.h"
 #include "Tab.h"
 #include "Utils.h"
-#include "winrt/Windows.UI.Popups.h"
 
 using namespace winrt::Windows::UI::Xaml;
 using namespace winrt::Windows::UI::Core;
@@ -34,7 +33,6 @@ Tab::Tab(const GUID& profile, const TermControl& control)
 void Tab::_MakeTabViewItem()
 {
     _tabViewItem = ::winrt::MUX::Controls::TabViewItem{};
-    _CreateContextMenu();
 }
 
 UIElement Tab::GetRootElement()
@@ -115,12 +113,27 @@ std::optional<GUID> Tab::GetFocusedProfile() const noexcept
 // - control: reference to the TermControl object to bind event to
 // Return Value:
 // - <none>
-void Tab::BindEventHandlers(const TermControl& control) noexcept
+void Tab::_BindEventHandlers(const TermControl& control) noexcept
 {
     _AttachEventHandlersToPane(_rootPane);
     _AttachEventHandlersToControl(control);
 }
 
+// Method Description:
+// - Called after construction of a Tab object to bind event handlers to its
+//   associated Pane and TermControl object and to create the context menu of
+//   the tab item
+// Arguments:
+// - control: reference to the TermControl object to bind event to
+// Return Value:
+// - <none>
+void Tab::Initialize(const TermControl& control)
+{
+    _BindEventHandlers(control);
+    _CreateContextMenu();
+}
+
+
 // Method Description:
 // - Attempts to update the settings of this tab's tree of panes.
 // Arguments:
@@ -396,20 +409,34 @@ void Tab::_CreateContextMenu()
     closeTabMenuItem.Text(tabClose);
     closeTabMenuItem.Icon(closeSymbol);
 
-    closeTabMenuItem.Click([this](auto&&, auto&&) {
-        ClosePane();
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    closeTabMenuItem.Click([weakThis](auto&&, auto&&) {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->ClosePane();
+        }
     });
 
-    _tabColorPickup.ColorSelected([this](auto newTabColor) {
-        _SetTabColor(newTabColor);
+    _tabColorPickup.ColorSelected([weakThis](auto newTabColor) {
+        if (auto tab{ weakThis.lock() })
+        {
+           tab->_SetTabColor(newTabColor);
+        }
     });
 
-    _tabColorPickup.ColorCleared([this]() {
-        _ResetTabColor();
+    _tabColorPickup.ColorCleared([weakThis]() {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_ResetTabColor();
+        }
     });
 
-    chooseColorMenuItem.Click([this](auto&&, auto&&) {
-        _tabColorPickup.ShowAt(_tabViewItem);
+    chooseColorMenuItem.Click([weakThis](auto&&, auto&&) {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_tabColorPickup.ShowAt(tab->_tabViewItem);
+        }
     });
     chooseColorMenuItem.Icon(colorPickSymbol);
 
@@ -432,12 +459,19 @@ void Tab::_CreateContextMenu()
 // - <none>
 void Tab::_SetTabColor(const winrt::Windows::UI::Color& color)
 {
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, color]() {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis, color]() {
+        auto ptrTab = weakThis.lock();
+        if (!ptrTab)
+            return;
+
+        auto tab{ ptrTab };
         Media::SolidColorBrush selectedTabBrush{};
         Media::SolidColorBrush deselectedTabBrush{};
         Media::SolidColorBrush fontBrush{};
 
-        // see https://en.wikipedia.org/wiki/Luma_(video)#Rec._601_luma_versus_Rec._709_luma_coefficients
+        // see https://www.w3.org/TR/WCAG20/#relativeluminancedef
         float c[] = { color.R / 255.f, color.G / 255.f, color.B / 255.f };
         for (int i = 0; i < 3; i++)
         {
@@ -470,23 +504,14 @@ void Tab::_SetTabColor(const winrt::Windows::UI::Color& color)
         auto deselectedTabColor = color;
         deselectedTabColor.A = 64;
         deselectedTabBrush.Color(deselectedTabColor);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundSelected"), selectedTabBrush);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackground"), deselectedTabBrush);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPointerOver"), selectedTabBrush);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForeground"), fontBrush);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundSelected"), fontBrush);
-        _tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundPointerOver"), fontBrush);
-        //switch the visual state so that the colors are updated immediately
-        if (_focused)
-        {
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
-        }
-        else
-        {
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
-        }
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundSelected"), selectedTabBrush);
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackground"), deselectedTabBrush);
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPointerOver"), selectedTabBrush);
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForeground"), fontBrush);
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundSelected"), fontBrush);
+        tab->_tabViewItem.Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundPointerOver"), fontBrush);
+
+        tab->_RefreshVisualState();
     });
 }
 
@@ -499,7 +524,14 @@ void Tab::_SetTabColor(const winrt::Windows::UI::Color& color)
 // - <none>
 void Tab::_ResetTabColor()
 {
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
+        auto ptrTab = weakThis.lock();
+        if (!ptrTab)
+            return;
+
+        auto tab{ ptrTab };
         winrt::hstring keys[] = {
             L"TabViewItemHeaderBackground",
             L"TabViewItemHeaderBackgroundSelected",
@@ -513,25 +545,36 @@ void Tab::_ResetTabColor()
         for (auto keyString : keys)
         {
             auto key = winrt::box_value(keyString);
-            if (_tabViewItem.Resources().HasKey(key))
+            if (tab->_tabViewItem.Resources().HasKey(key))
             {
-                _tabViewItem.Resources().Remove(key);
+                tab->_tabViewItem.Resources().Remove(key);
             }
         }
 
-        //switch the visual state so that the colors are updated
-        if (_focused)
-        {
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
-        }
-        else
-        {
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
-            winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
-        }
+        tab->_RefreshVisualState();
     });
 }
 
+// Method Description:
+// Toggles the visual state of the tab view item,
+// so that changes to the tab color are reflected immediately
+// Arguments:
+// - <none>
+// Return Value:
+// - <none>
+void Tab::_RefreshVisualState()
+{
+    if (_focused)
+    {
+        winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
+        winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
+    }
+    else
+    {
+        winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Selected", true);
+        winrt::Windows::UI::Xaml::VisualStateManager::GoToState(_tabViewItem, L"Normal", true);
+    }
+}
+
 UTILS_DEFINE_LIBRARY_RESOURCE_SCOPE(L"TerminalApp/Resources")
 DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>);
diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h
@@ -10,8 +10,8 @@ class Tab : public std::enable_shared_from_this<Tab>
 public:
     Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
 
-    // Called after construction to setup events with weak_ptr
-    void BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;
+    // Called after construction to perform the necessary setup, which relies on weak_ptr
+    void Initialize(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
 
     winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem();
     winrt::Windows::UI::Xaml::UIElement GetRootElement();
@@ -55,6 +55,9 @@ class Tab : public std::enable_shared_from_this<Tab>
     void _Focus();
     void _SetTabColor(const winrt::Windows::UI::Color& color);
     void _ResetTabColor();
+    void _RefreshVisualState();
+
+    void _BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;
 
     void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
     void _AttachEventHandlersToPane(std::shared_ptr<Pane> pane);

diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -795,7 +795,7 @@ namespace winrt::TerminalApp::implementation
         term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler });
 
         // Bind Tab events to the TermControl and the Tab's Pane
-        hostingTab->BindEventHandlers(term);
+        hostingTab->Initialize(term);
 
         // Don't capture a strong ref to the tab. If the tab is removed as this
         // is called, we don't really care anymore about handling the event.

diff --git a/src/cascadia/TerminalApp/lib/pch.h b/src/cascadia/TerminalApp/lib/pch.h
@@ -60,3 +60,4 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalAppProvider);
 #include <filesystem>
 
 #include <winrt/Microsoft.Terminal.TerminalConnection.h>
+#include <winrt/Windows.UI.Popups.h>