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

Clamp the focusTab action to the number of available tabs #10651

Merged
7 commits merged into from
Jul 22, 2021
Merged

Clamp the focusTab action to the number of available tabs #10651

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f1d98fb
Add a test for this case
zadjii-msft Jul 13, 2021
adca70d
I think this fixes #6776, but some tests are broken. They may have al…
zadjii-msft Jul 13, 2021
4dfa80e
Gosh, clamping is hard
zadjii-msft Jul 13, 2021
503b952
@dhowett wants it to go to the last tab? It'll go to the last tab
zadjii-msft Jul 14, 2021
25ed724
Merge remote-tracking branch 'origin/main' into dev/migrie/b/6776-foc…
zadjii-msft Jul 21, 2021
647577b
fine. make 'switch to last' a special case on UINT32_MAX
zadjii-msft Jul 21, 2021
131cef5
Update src/cascadia/LocalTests_TerminalApp/TabTests.cpp
zadjii-msft Jul 22, 2021
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
53 changes: 53 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ namespace TerminalAppLocalTests
TEST_METHOD(TestPreviewDismissScheme);
TEST_METHOD(TestPreviewSchemeWhilePreviewing);

TEST_METHOD(TestClampSwitchToTab);

TEST_CLASS_SETUP(ClassSetup)
{
return true;
Expand Down Expand Up @@ -1342,4 +1344,55 @@ namespace TerminalAppLocalTests
});
}

void TabTests::TestClampSwitchToTab()
{
Log::Comment(L"Preview a color scheme. Make sure it's applied, then committed accordingly");

zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
auto page = _commonSetup();
VERIFY_IS_NOT_NULL(page);

Log::Comment(L"Create a second tab");
TestOnUIThread([&page]() {
NewTerminalArgs newTerminalArgs{ 1 };
page->_OpenNewTab(newTerminalArgs);
});
VERIFY_ARE_EQUAL(2u, page->_tabs.Size());

Log::Comment(L"Create a third tab");
TestOnUIThread([&page]() {
NewTerminalArgs newTerminalArgs{ 2 };
page->_OpenNewTab(newTerminalArgs);
});
VERIFY_ARE_EQUAL(3u, page->_tabs.Size());

TestOnUIThread([&page]() {
auto focusedTabIndexOpt{ page->_GetFocusedTabIndex() };
VERIFY_IS_TRUE(focusedTabIndexOpt.has_value());
VERIFY_ARE_EQUAL(2u, focusedTabIndexOpt.value());
});

TestOnUIThread([&page]() {
Log::Comment(L"Switch to the first tab");
page->_SelectTab(0);
});

TestOnUIThread([&page]() {
auto focusedTabIndexOpt{ page->_GetFocusedTabIndex() };

VERIFY_IS_TRUE(focusedTabIndexOpt.has_value());
VERIFY_ARE_EQUAL(0u, focusedTabIndexOpt.value());
});

TestOnUIThread([&page]() {
Log::Comment(L"Switch to the tab 6, which is greater than number of tabs. This should switch to the third tab");
page->_SelectTab(6);
});

TestOnUIThread([&page]() {
auto focusedTabIndexOpt{ page->_GetFocusedTabIndex() };
VERIFY_IS_TRUE(focusedTabIndexOpt.has_value());
VERIFY_ARE_EQUAL(2u, focusedTabIndexOpt.value());
});
}

}
31 changes: 16 additions & 15 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,24 +497,25 @@ namespace winrt::TerminalApp::implementation
// TerminalPage::_OnTabSelectionChanged
// Return Value:
// true iff we were able to select that tab index, false otherwise
bool TerminalPage::_SelectTab(const uint32_t tabIndex)
bool TerminalPage::_SelectTab(uint32_t tabIndex)
{
if (tabIndex >= 0 && tabIndex < _tabs.Size())
{
auto tab{ _tabs.GetAt(tabIndex) };
if (_startupState == StartupState::InStartup)
{
_tabView.SelectedItem(tab.TabViewItem());
_UpdatedSelectedTab(tab);
}
else
{
_SetFocusedTab(tab);
}
// GH#9369 - if the argument is out of range, then clamp to the number
// of available tabs. Previously, we'd just silently do nothing if the
// value was greater than the number of tabs.
tabIndex = std::clamp(tabIndex, 0u, _tabs.Size() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

tabIndex can't be smaller than 0, so you can also use std::min.


return true;
auto tab{ _tabs.GetAt(tabIndex) };
if (_startupState == StartupState::InStartup)
{
_tabView.SelectedItem(tab.TabViewItem());
_UpdatedSelectedTab(tab);
}
else
{
_SetFocusedTab(tab);
}
return false;

return true;
}

// Method Description:
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 @@ -229,7 +229,7 @@ namespace winrt::TerminalApp::implementation
void _ResizeTabContent(const winrt::Windows::Foundation::Size& newSize);

void _SelectNextTab(const bool bMoveRight, const Windows::Foundation::IReference<Microsoft::Terminal::Settings::Model::TabSwitcherMode>& customTabSwitcherMode);
bool _SelectTab(const uint32_t tabIndex);
bool _SelectTab(uint32_t tabIndex);
void _MoveFocus(const Microsoft::Terminal::Settings::Model::FocusDirection& direction);

winrt::Microsoft::Terminal::Control::TermControl _GetActiveControl();
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/defaults.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@
{ "command": { "action": "switchToTab", "index": 5 }, "keys": "ctrl+alt+6" },
{ "command": { "action": "switchToTab", "index": 6 }, "keys": "ctrl+alt+7" },
{ "command": { "action": "switchToTab", "index": 7 }, "keys": "ctrl+alt+8" },
{ "command": { "action": "switchToTab", "index": 8 }, "keys": "ctrl+alt+9" },
{ "command": { "action": "switchToTab", "index": 99999 }, "keys": "ctrl+alt+9" },
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

// Pane Management
{ "command": "closePane", "keys": "ctrl+shift+w" },
Expand Down