Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update close button visibility based on BOTH settings and readonly mode #15914

Merged
merged 4 commits into from
Sep 6, 2023
Merged

Update close button visibility based on BOTH settings and readonly mode #15914

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c868bf
Update close button visibility based on BOTH settings and readonly mode
zadjii-msft Aug 31, 2023
5c421d0
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15902-re…
zadjii-msft Sep 5, 2023
88597fe
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15902-re…
zadjii-msft Sep 5, 2023
45f254b
comments make everyone happy
zadjii-msft Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions src/cascadia/TerminalApp/TabBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does changing the readonly state call this? I didn't see it in the diffs, but this function is new.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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

}

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:
{
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
TabViewItem().IsClosable(_focused());
break;
}
default:
TabViewItem().IsClosable(true);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}
}

bool TabBase::_focused() const noexcept
{
return _focusState != FocusState::Unfocused;
}

}
9 changes: 8 additions & 1 deletion src/cascadia/TerminalApp/TabBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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>);
Expand Down Expand Up @@ -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();

Expand All @@ -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;
};
}
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TabBase.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@ namespace winrt::TerminalApp::implementation
{
tab.Focus(FocusState::Programmatic);
_UpdateMRUTab(tab);
_updateAllTabCloseButtons(tab);
_updateAllTabCloseButtons();
}

tab.TabViewItem().StartBringIntoView();
Expand Down Expand Up @@ -1114,7 +1114,7 @@ namespace winrt::TerminalApp::implementation
{
tab.Focus(FocusState::Programmatic);
_UpdateMRUTab(tab);
_updateAllTabCloseButtons(tab);
_updateAllTabCloseButtons();
}
}
}
Expand Down
33 changes: 6 additions & 27 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/TerminalTab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ namespace winrt::TerminalApp::implementation

_focusState = focusState;

if (_focusState != FocusState::Unfocused)
if (_focused())
{
auto lastFocusedControl = GetActiveTerminalControl();
if (lastFocusedControl)
Expand Down Expand Up @@ -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>() })
{
Expand Down Expand Up @@ -1693,7 +1693,7 @@ namespace winrt::TerminalApp::implementation
}

ReadOnly(_rootPane->ContainsReadOnly());
TabViewItem().IsClosable(!ReadOnly());
_updateIsClosable();
}

std::shared_ptr<Pane> TerminalTab::GetActivePane() const
Expand Down
Loading