From 8c868bf8095e859e1cc86156d1ee9321898d5bf4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 31 Aug 2023 10:19:42 -0500 Subject: [PATCH 1/2] Update close button visibility based on BOTH settings and readonly mode `TerminalTab::_RecalculateAndApplyReadOnly` didn't know about whether a tab should be closable or not, based on the theme settings. Similarly (though, unreported), the theme update in `TerminalPage::_updateAllTabCloseButtons` didn't really know about readonly mode. This fixes both these issues by moving responsibility for the tab close button visibility into `TabBase` itself. Closes #15902 --- src/cascadia/TerminalApp/TabBase.cpp | 49 ++++++++++++++++++++-- src/cascadia/TerminalApp/TabBase.h | 9 +++- src/cascadia/TerminalApp/TabBase.idl | 2 +- src/cascadia/TerminalApp/TabManagement.cpp | 4 +- src/cascadia/TerminalApp/TerminalPage.cpp | 33 +++------------ src/cascadia/TerminalApp/TerminalPage.h | 2 +- src/cascadia/TerminalApp/TerminalTab.cpp | 6 +-- 7 files changed, 66 insertions(+), 39 deletions(-) diff --git a/src/cascadia/TerminalApp/TabBase.cpp b/src/cascadia/TerminalApp/TabBase.cpp index 2d7293ccd6d..fc0a1198305 100644 --- a/src/cascadia/TerminalApp/TabBase.cpp +++ b/src/cascadia/TerminalApp/TabBase.cpp @@ -25,10 +25,6 @@ namespace winrt namespace winrt::TerminalApp::implementation { - WUX::FocusState TabBase::FocusState() const noexcept - { - return _focusState; - } // Method Description: // - Prepares this tab for being removed from the UI hierarchy @@ -590,4 +586,49 @@ namespace winrt::TerminalApp::implementation VisualStateManager::GoToState(item, L"Normal", true); } } + + TabCloseButtonVisibility TabBase::CloseButtonVisibility() + { + return _closeButtonVisibility; + } + + void TabBase::CloseButtonVisibility(TabCloseButtonVisibility visibility) + { + _closeButtonVisibility = visibility; + _updateIsClosable(); + } + + void TabBase::_updateIsClosable() + { + if (ReadOnly()) + { + TabViewItem().IsClosable(false); + } + else + { + switch (_closeButtonVisibility) + { + case TabCloseButtonVisibility::Never: + TabViewItem().IsClosable(false); + break; + case TabCloseButtonVisibility::Hover: + TabViewItem().IsClosable(true); + break; + case TabCloseButtonVisibility::ActiveOnly: + { + TabViewItem().IsClosable(_focused()); + break; + } + default: + TabViewItem().IsClosable(true); + break; + } + } + } + + bool TabBase::_focused() const noexcept + { + return _focusState != FocusState::Unfocused; + } + } diff --git a/src/cascadia/TerminalApp/TabBase.h b/src/cascadia/TerminalApp/TabBase.h index 24ac6d2da6f..108b30a502c 100644 --- a/src/cascadia/TerminalApp/TabBase.h +++ b/src/cascadia/TerminalApp/TabBase.h @@ -16,7 +16,6 @@ namespace winrt::TerminalApp::implementation { public: virtual void Focus(winrt::Windows::UI::Xaml::FocusState focusState) = 0; - winrt::Windows::UI::Xaml::FocusState FocusState() const noexcept; virtual void Shutdown(); void SetDispatch(const winrt::TerminalApp::ShortcutActionDispatch& dispatch); @@ -30,6 +29,9 @@ namespace winrt::TerminalApp::implementation const winrt::Microsoft::Terminal::Settings::Model::ThemeColor& unfocused, const til::color& tabRowColor); + Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility CloseButtonVisibility(); + void CloseButtonVisibility(Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility visible); + WINRT_CALLBACK(RequestFocusActiveControl, winrt::delegate); WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); @@ -60,6 +62,8 @@ namespace winrt::TerminalApp::implementation winrt::Microsoft::Terminal::Settings::Model::ThemeColor _unfocusedThemeColor{ nullptr }; til::color _tabRowColor; + Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility _closeButtonVisibility{ Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility::Always }; + virtual void _CreateContextMenu(); virtual winrt::hstring _CreateToolTipTitle(); @@ -78,6 +82,9 @@ namespace winrt::TerminalApp::implementation void _RefreshVisualState(); virtual winrt::Windows::UI::Xaml::Media::Brush _BackgroundBrush() = 0; + bool _focused() const noexcept; + void _updateIsClosable(); + friend class ::TerminalAppLocalTests::TabTests; }; } diff --git a/src/cascadia/TerminalApp/TabBase.idl b/src/cascadia/TerminalApp/TabBase.idl index a8406a5c51e..20ee8ffa9d0 100644 --- a/src/cascadia/TerminalApp/TabBase.idl +++ b/src/cascadia/TerminalApp/TabBase.idl @@ -9,10 +9,10 @@ namespace TerminalApp String Title { get; }; String Icon { get; }; Boolean ReadOnly { get; }; + Microsoft.Terminal.Settings.Model.TabCloseButtonVisibility CloseButtonVisibility { get; set; }; Microsoft.UI.Xaml.Controls.TabViewItem TabViewItem { get; }; Windows.UI.Xaml.FrameworkElement Content { get; }; - Windows.UI.Xaml.FocusState FocusState { get; }; UInt32 TabViewIndex; UInt32 TabViewNumTabs; diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index 0d8bcbee26f..02bd8940746 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -936,7 +936,7 @@ namespace winrt::TerminalApp::implementation { tab.Focus(FocusState::Programmatic); _UpdateMRUTab(tab); - _updateAllTabCloseButtons(tab); + _updateAllTabCloseButtons(); } tab.TabViewItem().StartBringIntoView(); @@ -1114,7 +1114,7 @@ namespace winrt::TerminalApp::implementation { tab.Focus(FocusState::Programmatic); _UpdateMRUTab(tab); - _updateAllTabCloseButtons(tab); + _updateAllTabCloseButtons(); } } } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 024ae18c10a..b96111994d9 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -3283,44 +3283,23 @@ namespace winrt::TerminalApp::implementation // Begin Theme handling _updateThemeColors(); - _updateAllTabCloseButtons(_GetFocusedTab()); + _updateAllTabCloseButtons(); } - void TerminalPage::_updateAllTabCloseButtons(const winrt::TerminalApp::TabBase& focusedTab) + void TerminalPage::_updateAllTabCloseButtons() { // Update the state of the CloseButtonOverlayMode property of // our TabView, to match the tab.showCloseButton property in the theme. // // Also update every tab's individual IsClosable to match the same property. const auto theme = _settings.GlobalSettings().CurrentTheme(); - const auto visibility = theme && theme.Tab() ? theme.Tab().ShowCloseButton() : Settings::Model::TabCloseButtonVisibility::Always; + const auto visibility = (theme && theme.Tab()) ? + theme.Tab().ShowCloseButton() : + Settings::Model::TabCloseButtonVisibility::Always; for (const auto& tab : _tabs) { - switch (visibility) - { - case Settings::Model::TabCloseButtonVisibility::Never: - tab.TabViewItem().IsClosable(false); - break; - case Settings::Model::TabCloseButtonVisibility::Hover: - tab.TabViewItem().IsClosable(true); - break; - case Settings::Model::TabCloseButtonVisibility::ActiveOnly: - { - if (focusedTab && focusedTab == tab) - { - tab.TabViewItem().IsClosable(true); - } - else - { - tab.TabViewItem().IsClosable(false); - } - break; - } - default: - tab.TabViewItem().IsClosable(true); - break; - } + tab.CloseButtonVisibility(visibility); } switch (visibility) diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index fd63f7cc674..cb82efeaa8a 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -511,7 +511,7 @@ namespace winrt::TerminalApp::implementation static void _DismissMessage(const winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage& message); void _updateThemeColors(); - void _updateAllTabCloseButtons(const winrt::TerminalApp::TabBase& focusedTab); + void _updateAllTabCloseButtons(); void _updatePaneResources(const winrt::Windows::UI::Xaml::ElementTheme& requestedTheme); winrt::fire_and_forget _ControlCompletionsChangedHandler(const winrt::Windows::Foundation::IInspectable sender, const winrt::Microsoft::Terminal::Control::CompletionsChangedEventArgs args); diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index 1b881029b56..864ff6aad4e 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -223,7 +223,7 @@ namespace winrt::TerminalApp::implementation _focusState = focusState; - if (_focusState != FocusState::Unfocused) + if (_focused()) { auto lastFocusedControl = GetActiveTerminalControl(); if (lastFocusedControl) @@ -961,7 +961,7 @@ namespace winrt::TerminalApp::implementation co_await wil::resume_foreground(dispatcher); if (const auto tab{ weakThis.get() }) { - if (tab->_focusState != FocusState::Unfocused) + if (tab->_focused()) { if (const auto termControl{ sender.try_as() }) { @@ -1693,7 +1693,7 @@ namespace winrt::TerminalApp::implementation } ReadOnly(_rootPane->ContainsReadOnly()); - TabViewItem().IsClosable(!ReadOnly()); + _updateIsClosable(); } std::shared_ptr TerminalTab::GetActivePane() const From 45f254b4d986ec3be12146097af54d57c00f4103 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 5 Sep 2023 12:07:40 -0500 Subject: [PATCH 2/2] comments make everyone happy --- src/cascadia/TerminalApp/TabBase.cpp | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalApp/TabBase.cpp b/src/cascadia/TerminalApp/TabBase.cpp index fc0a1198305..5077067762a 100644 --- a/src/cascadia/TerminalApp/TabBase.cpp +++ b/src/cascadia/TerminalApp/TabBase.cpp @@ -592,38 +592,52 @@ namespace winrt::TerminalApp::implementation return _closeButtonVisibility; } + // Method Description: + // - set our internal state to track if we were requested to have a visible + // tab close button or not. + // - This is called every time the active tab changes. That way, the changes + // in focused tab can be reflected for the "ActiveOnly" state. void TabBase::CloseButtonVisibility(TabCloseButtonVisibility visibility) { _closeButtonVisibility = visibility; _updateIsClosable(); } + // Method Description: + // - Update our close button's visibility, to reflect both the ReadOnly + // state of the tab content, and also if if we were told to have a visible + // close button at all. + // - the tab being read-only takes precedence. That will always suppress + // the close button. + // - Otherwise we'll use the state set in CloseButtonVisibility to control + // the tab's visibility. void TabBase::_updateIsClosable() { + bool isClosable = true; + if (ReadOnly()) { - TabViewItem().IsClosable(false); + isClosable = false; } else { switch (_closeButtonVisibility) { case TabCloseButtonVisibility::Never: - TabViewItem().IsClosable(false); + isClosable = false; break; case TabCloseButtonVisibility::Hover: - TabViewItem().IsClosable(true); + isClosable = true; break; case TabCloseButtonVisibility::ActiveOnly: - { - TabViewItem().IsClosable(_focused()); + isClosable = _focused(); break; - } default: - TabViewItem().IsClosable(true); + isClosable = true; break; } } + TabViewItem().IsClosable(isClosable); } bool TabBase::_focused() const noexcept