Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
19 changes: 19 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(SwapPanes);

TEST_METHOD(NextMRUTab);
TEST_METHOD(TabRowVisibilityReflectsTabCount);
TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder);

TEST_METHOD(TestWindowRenameSuccessful);
Expand Down Expand Up @@ -1144,6 +1145,24 @@ namespace TerminalAppLocalTests
});
}

void TabTests::TabRowVisibilityReflectsTabCount()
{
auto page = _commonSetup();

TestOnUIThread([&page]() {
VERIFY_IS_FALSE(page->IsTabRowVisible(), L"Single tab with tabs hidden in title bar should not show the tab row.");
});

TestOnUIThread([&page]() {
NewTerminalArgs newTerminalArgs{ 1 };
page->_OpenNewTab(newTerminalArgs);
});

TestOnUIThread([&page]() {
VERIFY_IS_TRUE(page->IsTabRowVisible(), L"Multiple tabs should make the tab row visible when tabs are outside the title bar.");
});
}

void TabTests::VerifyCommandPaletteTabSwitcherOrder()
{
// This is a test for GH#8188 - we want to make sure that the order of tabs
Expand Down
6 changes: 1 addition & 5 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,7 @@ namespace winrt::TerminalApp::implementation
// - we're not in focus mode
// - we're not in full screen, or the user has enabled fullscreen tabs
// - there is more than one tab, or the user has chosen to always show tabs
const auto isVisible = !_isInFocusMode &&
(!_isFullscreen || _showTabsFullscreen) &&
(_settings.GlobalSettings().ShowTabsInTitlebar() ||
(_tabs.Size() > 1) ||
_settings.GlobalSettings().AlwaysShowTabs());
const auto isVisible = IsTabRowVisible();

if (_tabView)
{
Expand Down
14 changes: 14 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4210,6 +4210,20 @@ namespace winrt::TerminalApp::implementation
return _isAlwaysOnTop;
}

bool TerminalPage::IsTabRowVisible() const
{
if (!_settings)
{
return false;
}

return !_isInFocusMode &&
(!_isFullscreen || _showTabsFullscreen) &&
(_settings.GlobalSettings().ShowTabsInTitlebar() ||
NumberOfTabs() > 1 ||
_settings.GlobalSettings().AlwaysShowTabs());
}

// Method Description:
// - Returns true if the tab row should be visible when we're in full screen
// state.
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ namespace winrt::TerminalApp::implementation
bool FocusMode() const;
bool Fullscreen() const;
bool AlwaysOnTop() const;
bool IsTabRowVisible() const;
bool ShowTabsFullscreen() const;
void SetShowTabsFullscreen(bool newShowTabsFullscreen);
void SetFullscreen(bool);
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.idl
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ namespace TerminalApp
Boolean Fullscreen { get; };
Boolean AlwaysOnTop { get; };

Boolean IsTabRowVisible();

WindowProperties WindowProperties { get; };
void IdentifyWindow();

Expand Down
43 changes: 27 additions & 16 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1316,26 +1316,37 @@ namespace winrt::TerminalApp::implementation

if (!FocusMode())
{
if (!_settings.GlobalSettings().AlwaysShowTabs())
const auto tabsInTitlebar = _settings.GlobalSettings().ShowTabsInTitlebar();
const auto tabRowVisible = _root ? _root->IsTabRowVisible() :
(_settings.GlobalSettings().AlwaysShowTabs() || tabsInTitlebar);

if (!tabsInTitlebar)
{
// Hide the title bar = off, Always show tabs = off.
static constexpr auto titlebarHeight = 10;
pixelSize.Height += (titlebarHeight)*scale;
if (tabRowVisible)
{
// Hide the title bar = off, tab row visible. Account for
// both the system title bar and the tab strip height.
static constexpr auto titlebarAndTabBarHeight = 40;
pixelSize.Height += (titlebarAndTabBarHeight)*scale;
}
else
{
// Hide the title bar = off, tab row hidden. Only account
// for the native title bar height.
static constexpr auto titlebarHeight = 10;
pixelSize.Height += (titlebarHeight)*scale;
}
}
else if (!_settings.GlobalSettings().ShowTabsInTitlebar())
else if (tabRowVisible)
{
// Hide the title bar = off, Always show tabs = on.
static constexpr auto titlebarAndTabBarHeight = 40;
pixelSize.Height += (titlebarAndTabBarHeight)*scale;
// Hide the title bar = on, tabs drawn in the title bar. Add
// the custom tab row height so the client area fits.
static constexpr auto titlebarHeight = 40;
pixelSize.Height += (titlebarHeight)*scale;
Copy link
Member

Choose a reason for hiding this comment

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

(Sorry to push to your branch before making this comment - the line endings were complicating the review!)

It looks like the code here on R1327 is the same as R1342.

It looks like it might be possible to consolidate them by flipping around the order in which we check:

if (tabRowVisible) {
    // it's 40
} else {
    if (!tabsInTitlebar) {
        // it's 10 apparently
    }
}

but I can't currently determine that that is identical. thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

No worries on the branch push! I'll try and consolidate this part of the code.

Copy link
Author

Choose a reason for hiding this comment

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

@DHowett just pushed the commit to merge this logic, let me know what you think.

}
// Hide the title bar = on, Always show tabs = on.
// In this case, we don't add any height because
// NonClientIslandWindow::GetTotalNonClientExclusiveSize() gets
// called in AppHost::_resizeWindow and it already takes title bar
// height into account. In other cases above
// IslandWindow::GetTotalNonClientExclusiveSize() is called, and it
// doesn't take the title bar height into account, so we have to do
// the calculation manually.
// When tabs are hosted in the title bar but not visible we don't
// need to adjust the size – the non-client window chrome already
// accounts for it.
}

args.Width(static_cast<int32_t>(pixelSize.Width));
Expand Down