Repository: microsoft/terminal (language: cpp)

Issue:
Pane movement breaks showCloseButton=never
### Windows Terminal version

1.17.11461.0

### Windows build number

Version 10.0.19045.3324

### Other Software

_No response_

### Steps to reproduce

Add the following to `settings.json` (in `%LOCALAPPDATA%\Packages\Microsoft.WindowsTerminal_8wekyb3d8bbwe\LocalState\settings.json`):
```json
"theme": "foo",
"themes": [
  {
    "name": "foo",
    "tab": {
      "showCloseButton": "never"
    }
  }
]
```

Run in Command Prompt:

```shell
wt -M -p "Ubuntu-20.04" ; ^
	split-pane -p "Ubuntu-20.04" ;
```

Observe that the first tab has an "X" on the right that you can click to close the tab. But, `tab.showCloseButton`: `never` should have hidden this "X". The `split-pane` line is causing this bug. If you remove it, i.e.

```shell
wt -M -p "Ubuntu-20.04" ;
```

then the tab no longer has the "X", as expected

For related discussion:
- https://github.com/microsoft/terminal/issues/3335
- https://github.com/microsoft/terminal/pull/13348

### Expected Behavior

The "X" icon should not be present on a Windows Terminal tab if `tab.showCloseButton`: `never`

### Actual Behavior

The "X" icon is present on a tab if using `split-pane` in `wt`

---

_Edit (lhecker)_: **Any movement between two panes makes the buttons reappear**.


Discussion / hints:
Thanks for filing! We found a more general issue with `showCloseButton` thanks to you :smile:
Oh well that'll do it:

* `TerminalPage::_updateAllTabCloseButtons` is where we apply `TabViewItem().IsClosable` based off the theme
* Presumably, `TerminalTab::_RecalculateAndApplyReadOnly` fires later, which then updates the tab's own item's `IsClosable` based on read-only state only. 

Fix patch:
diff --git a/src/cascadia/TerminalApp/TabBase.cpp b/src/cascadia/TerminalApp/TabBase.cpp
index 2d7293ccd6d..5077067762a 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,63 @@ namespace winrt::TerminalApp::implementation
             VisualStateManager::GoToState(item, L"Normal", true);
         }
     }
+
+    TabCloseButtonVisibility TabBase::CloseButtonVisibility()
+    {
+        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())
+        {
+            isClosable = false;
+        }
+        else
+        {
+            switch (_closeButtonVisibility)
+            {
+            case TabCloseButtonVisibility::Never:
+                isClosable = false;
+                break;
+            case TabCloseButtonVisibility::Hover:
+                isClosable = true;
+                break;
+            case TabCloseButtonVisibility::ActiveOnly:
+                isClosable = _focused();
+                break;
+            default:
+                isClosable = true;
+                break;
+            }
+        }
+        TabViewItem().IsClosable(isClosable);
+    }
+
+    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<void()>);
 
         WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
@@ -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<winrt::Microsoft::Terminal::Control::TermControl>() })
                     {
@@ -1693,7 +1693,7 @@ namespace winrt::TerminalApp::implementation
         }
 
         ReadOnly(_rootPane->ContainsReadOnly());
-        TabViewItem().IsClosable(!ReadOnly());
+        _updateIsClosable();
     }
 
     std::shared_ptr<Pane> TerminalTab::GetActivePane() const


Test patch: