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

## Summary of the Pull Request When we perform a `focusTab` action, we currently do nothing if the parameter was greater than the number of tabs. This PR changes that behavior. Now, `focus-tab -t 999999` will always focus the last tab, instead of silently doing nothing. ## PR Checklist * [x] Closes #9369 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * [x] ran tests * [x] validated commandline manually
microsoft · Jul 22, 2021 · 335f69e · 335f69e
1 parent cf97a9f
commit 335f69e
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 17 deletions.
diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp
@@ -95,6 +95,8 @@ namespace TerminalAppLocalTests
         TEST_METHOD(TestPreviewDismissScheme);
         TEST_METHOD(TestPreviewSchemeWhilePreviewing);
 
+        TEST_METHOD(TestClampSwitchToTab);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             return true;
@@ -1550,4 +1552,55 @@ namespace TerminalAppLocalTests
         });
     }
 
+    void TabTests::TestClampSwitchToTab()
+    {
+        Log::Comment(L"Test that switching to a tab index higher than the number of tabs just clamps to the last tab.");
+
+        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());
+        });
+    }
+
 }
diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp
@@ -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);
 
-            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:

diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h
@@ -233,7 +233,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);
         void _MovePane(const Microsoft::Terminal::Settings::Model::FocusDirection& direction);
 

diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionArgs.cpp
@@ -228,6 +228,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
     winrt::hstring SwitchToTabArgs::GenerateName() const
     {
+        if (TabIndex() == UINT32_MAX)
+        {
+            return RS_(L"SwitchToLastTabCommandKey");
+        }
+
         return winrt::hstring{
             fmt::format(L"{}, index:{}", RS_(L"SwitchToTabCommandKey"), TabIndex())
         };

diff --git a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
@@ -365,6 +365,9 @@
   <data name="SwitchToTabCommandKey" xml:space="preserve">
     <value>Switch to tab</value>
   </data>
+  <data name="SwitchToLastTabCommandKey" xml:space="preserve">
+    <value>Switch to the last tab</value>
+  </data>
   <data name="TabSearchCommandKey" xml:space="preserve">
     <value>Search for tab...</value>
   </data>

diff --git a/src/cascadia/TerminalSettingsModel/defaults.json b/src/cascadia/TerminalSettingsModel/defaults.json
@@ -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": 4294967295 }, "keys": "ctrl+alt+9" },
 
         // Pane Management
         { "command": "closePane", "keys": "ctrl+shift+w" },