Update close button visibility based on BOTH settings and readonly mode #15914
Conversation
`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
microsoft-github-policy-service
bot
added
Issue-Bug
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
| void TabBase::CloseButtonVisibility(TabCloseButtonVisibility visibility) | ||
| { | ||
| _closeButtonVisibility = visibility; | ||
| _updateIsClosable(); |
There was a problem hiding this comment.
Does changing the readonly state call this? I didn't see it in the diffs, but this function is new.
There was a problem hiding this comment.
Oh, I don't know how I like that. ReadOnly doesn't directly impact whether it's closeable unless it's a terminal tab and ReadOnly was set by recalculation.
Shouldn't setting ReadOnly automatically trigger this?
There was a problem hiding this comment.
TerminalTab::_RecalculateAndApplyReadOnly calls into _updateIsClosable. It doesn't need to set CloseButtonVisibility itself. Think of CloseButtonVisibility as "the theme may or may not want the tab's close button visible", and readonly is a temporary override to that state.
This PR just disentagles the two, so they're not both just mucking with the IsClosable state directly.
There was a problem hiding this comment.
No, it absolutely should not call CloseButtonVisibility! What I mean is, shouldn't setting ReadOnly automatically call _updateIsClosable? You've disentangled them, but you left a thread dangling over in some other function that randomly has to know to call _updateIsClosable.
There was a problem hiding this comment.
_RecalculateAndApplyReadOnly gets called any time the active pane changes, or any time a control's ReadOnly state changes. So setting ReadOnly on the pane will set it on the control, who will tell the tab, who will then call _RecalculateAndApplyReadOnly
…adonly-close-never
…adonly-close-never
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
…de (#15914) `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 (cherry picked from commit 2f41d23) Service-Card-Id: 90379173 Service-Version: 1.18
TerminalTab::_RecalculateAndApplyReadOnlydidn't know about whether a tab should be closable or not, based on the theme settings. Similarly (though, unreported), the theme update inTerminalPage::_updateAllTabCloseButtonsdidn't really know about readonly mode.This fixes both these issues by moving responsibility for the tab close button visibility into
TabBaseitself.Closes #15902