Repository: microsoft/terminal (language: cpp)

Issue:
Next and Prev tab command palette actions can only focus the first tab
### Windows Terminal version (or Windows build number)

1.11.2421 (I haven't tried 1.10 to see if it is also present there)

### Other Software

_No response_

### Steps to reproduce

have more than one tab, attempt to move to a tab that isn't the first using the command palette.

### Expected Behavior

The desired tab should be focused.

### Actual Behavior

Only the first tab can be focused.


https://user-images.githubusercontent.com/6185249/132109193-8cf8961b-cecb-4414-8264-cfe57a51fd30.mp4




Discussion / hints:
_Huh._ That's not awesome. Thanks for filing!
Hmmm. I'm not seeing this on 
* `main`
* 1.10.2383.0
* 1.12.2712.0

So maybe it was only temporarily broken. @Rosefield Are you still seeing this on 1.11, but not on dev builds? Arguable, this should be fixed for when 1.11 goes to stable, but we'd need to find the commit that actually fixes this to backport that...

EDIT: I also can't repro with `Windows Terminal (Unpackaged) Version: 1.11.210830001-release1.11`
Tested on my laptop that does not have dev builds installed at all, and I see it on the preview build still. To be clear, I also see this on the dev builds with current `main`.

![image](https://user-images.githubusercontent.com/6185249/135305220-83e661a7-2bf2-4de3-802b-0e2da7a170e6.png)

and 

![image](https://user-images.githubusercontent.com/6185249/135305369-a28122d5-0103-4410-9c7f-94097000a44b.png)

Oh derp. I was using the actual "Switch to tab, index: whatever" action. That works, but yea the next/prev ones don't.

This DOES repro on 1.10 as well.


My guess is that this might have repro'd for at least 8 months.

https://github.com/microsoft/terminal/blame/8470857016e910ca275cd9f25daa20e57754ca8f/src/cascadia/TerminalApp/TerminalPage.cpp#L1546

Working theory: 
* We close the command palette. 
* We dispatch a "next tab" action
* That automatically uses the command palette to move to the next MRU tab
* No keys are pressed, so we immediately dismiss the command palette again, _selecting the current tab_.

I'll have to debug through to see if that's exactly what's going on here, but considering this has been around for months, I'm gonna take it out of the 1.12 fire queue.



Slapping some logging in:
```
CommandPalette::_close
CommandPalette::_close
_SelectNextTab 0x00000002 <----- here we dispatch the Next Tab action from tab 2.
_OnSwitchToTabRequested 0x00000000 <----- when we open the cmdpal, it requests that we switch to tab 0 immediately
_SelectTab: 0x00000000 
_OnSwitchToTabRequested 0x00000002 <---- then we actually switch to tab 2 (where we started)
_SelectTab: 0x00000002
CommandPalette::_dispatchCommand...._switchToTab
_OnSwitchToTabRequested 0x00000002
_SelectTab: 0x00000002
CommandPalette::_close
CommandPalette::_close
CommandPalette::_close
_OnSwitchToTabRequested 0x00000000 <---- This spurious late _OnSwitchToTabRequested is the one that's requesting we switch to index 0 for some reason.
_SelectTab: 0x00000000
```



That last `_close` is coming from a 

```
 	TerminalApp.dll!winrt::TerminalApp::implementation::TerminalPage::_OnSwitchToTabRequested(const winrt::Windows::Foundation::IInspectable & __formal, const winrt::TerminalApp::TabBase & tab) Line 641	C++
 	TerminalApp.dll!<lambda_b66858e62ce801dd3f7eefa302c934e6>::operator()<winrt::TerminalApp::CommandPalette const &,winrt::TerminalApp::TabBase const &>(const winrt::TerminalApp::CommandPalette & <args_0>, const winrt::TerminalApp::TabBase & <args_1>) Line 2512	C++
 	TerminalApp.dll!winrt::impl::delegate<winrt::Windows::Foundation::TypedEventHandler<winrt::TerminalApp::CommandPalette,winrt::TerminalApp::TabBase>,<lambda_b66858e62ce801dd3f7eefa302c934e6>>::Invoke(void * sender, void * args) Line 895	C++
 	TerminalApp.dll!winrt::Windows::Foundation::TypedEventHandler<winrt::TerminalApp::CommandPalette,winrt::TerminalApp::TabBase>::operator()(const winrt::TerminalApp::CommandPalette & sender, const winrt::TerminalApp::TabBase & args) Line 2525	C++
 	TerminalApp.dll!winrt::impl::invoke<winrt::Windows::Foundation::TypedEventHandler<winrt::TerminalApp::CommandPalette,winrt::TerminalApp::TabBase>,winrt::TerminalApp::implementation::CommandPalette,winrt::TerminalApp::TabBase>(const winrt::Windows::Foundation::TypedEventHandler<winrt::TerminalApp::CommandPalette,winrt::TerminalApp::TabBase> & delegate, const winrt::TerminalApp::implementation::CommandPalette & <args_0>, const winrt::TerminalApp::TabBase & <args_1>) Line 5762	C++
 	TerminalApp.dll!winrt::event<winrt::Windows::Foundation::TypedEventHandler<winrt::TerminalApp::CommandPalette,winrt::TerminalApp::TabBase>>::operator()<winrt::TerminalApp::implementation::CommandPalette,winrt::TerminalApp::TabBase>(const winrt::TerminalApp::implementation::CommandPalette & <args_0>, const winrt::TerminalApp::TabBase & <args_1>) Line 5897	C++
 	TerminalApp.dll!winrt::TerminalApp::implementation::CommandPalette::_switchToTab(const winrt::TerminalApp::FilteredCommand & filteredCommand) Line 730	C++
 	TerminalApp.dll!winrt::TerminalApp::implementation::CommandPalette::_selectedCommandChanged(const winrt::Windows::Foundation::IInspectable & __formal, const winrt::Windows::UI::Xaml::RoutedEventArgs & __formal) Line 239	C++
 	TerminalApp.dll!<lambda_ee9ff3fa2e15a47b43548da8d43a8eb3>::operator()<winrt::Windows::Foundation::IInspectable const &,winrt::Windows::UI::Xaml::Controls::SelectionChangedEventArgs const &>(const winrt::Windows::Foundation::IInspectable & <args_0>, const winrt::Windows::UI::Xaml::Controls::SelectionChangedEventArgs & <args_1>) Line 64144	C++
 	TerminalApp.dll!winrt::impl::delegate<winrt::Windows::UI::Xaml::Controls::SelectionChangedEventHandler,<lambda_ee9ff3fa2e15a47b43548da8d43a8eb3>>::Invoke(void * sender, void * e) Line 22397	C++
 	[External Code]	
 	TerminalApp.dll!winrt::impl::consume_Windows_UI_Xaml_Controls_Primitives_ISelector<winrt::Windows::UI::Xaml::Controls::ListView>::SelectedIndex(int value) Line 3112	C++
>	TerminalApp.dll!winrt::TerminalApp::implementation::CommandPalette::_filterTextChanged(const winrt::Windows::Foundation::IInspectable & __formal, const winrt::Windows::UI::Xaml::RoutedEventArgs & __formal) Line 819	C++
 	[External Code]	
 	TerminalApp.dll!winrt::impl::delegate<winrt::Windows::UI::Xaml::Controls::TextChangedEventHandler,void <lambda>(const winrt::Windows::Foundation::IInspectable &, const winrt::Windows::UI::Xaml::Controls::TextChangedEventArgs &)>::Invoke(void * sender, void * e) Line 22419	C++
 	[External Code]	
 	WindowsTerminal.exe!wWinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, wchar_t * __formal, int __formal) Line 155	C++

```

which looks like the `TextChanged` is firing. I wonder...
Alright I've pushed my experimentation in 84dd8ca66. In that commit, these actions work in the command palette ~, but they seemingly don't update the MRU order when invoked from the command palette. Maybe that's okay? I'm too afraid to touch that more, I don't~

Actually, I forgot the default tab switcher order is in-order. So this seems like it might work though. Future me can double check this when we decide to come back to this, since it's not blocking anymore.
I'm also seeing this issue on `v1.13.11431.0` and the [previous comment](https://github.com/microsoft/terminal/issues/11146#issuecomment-930379473) from almost a year ago is not completely clear to me, so I'm asking:

* Has this been fixed but not released yet?
* If yes: In what version is this meant to be released and/or is there an ETA release date?
* If no: Is this being worked on or stale?

Thanks!
Sorry, my previous comment was definitely not super clear there 😅 

* Nope, I had a test branch where I was investigating, but never ended up finishing that investigation
* n/a
* Nah, this isn't being worked on currently. If you'd like to help here, I think the commit I mentioned above might work, we just need to re-validate that and also check with the various `tabSwitcherMode` setting values to ensure they all still work. 
I have no intention of participating myself here ATM but your info might be interesting for someone else.
Thanks for the update 👍!
Hey @Rosefield @CombeeMike I was able to reproduce this on Windows 10 Terminal with this version of the terminal 

![image](https://github.com/microsoft/terminal/assets/40709873/d0bc2c20-260b-43e1-9d30-d8963ceec794)

I'm looking to contribute to this project, and I would like to know if I can pick this issue to start with. Thanks

@AlejandroBlanco2001 

I'm on version `1.19.10573.0` and am also still seeing this issue, so I'd love to see it fixed.
However, I have not contributed to terminal myself so I can't tell you whether or not your contribution would be welcome 🤷‍♂️.
@zadjii-msft, I conducted an exploratory test and familiarized myself with the codebase. In some sense, I can corroborate your point in this [comment](https://github.com/microsoft/terminal/issues/11146#issuecomment-930322440). I observed that when performing the following actions:

1. With an empty command line, selecting "Next Tab" with the mouse does not trigger this issue.
2. Pressing the keyboard command `Ctrl+Tab` also does not cause the issue.
3. Moving with the arrow keys likewise does not cause the issue.

However, while debugging this, I noticed that when typing something in the console and then selecting "Next tab," the `_filterTextChanged` function is called twice. It seems that the code is executing as follows:

Imagine we have 3 tabs, and we are currently on the first one (index 0):
1. Calling `_SelectNextTab` to move from 0 to 1, where the value of `bMoveRight` depends on the action.
2. Loading the command palette and triggering `SelectNextItem`.
3. Checking if it can move and then moving left/right.
4. It moves to the next/previous tab.
5. `_filterTextChanged` is called again, resetting the index to 0.

```
_filteredActionsView().SelectedIndex(_currentMode == CommandPaletteMode::CommandlineMode ? -1 : 0);
```

I will investigate why this is being called twice and prevent that double call. I will then share a possible solution and if is ok I will submit a PR.

Fix patch:
diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp
index dfba90a40ae..81921210c29 100644
--- a/src/cascadia/TerminalApp/CommandPalette.cpp
+++ b/src/cascadia/TerminalApp/CommandPalette.cpp
@@ -843,6 +843,16 @@ namespace winrt::TerminalApp::implementation
     void CommandPalette::_filterTextChanged(const IInspectable& /*sender*/,
                                             const Windows::UI::Xaml::RoutedEventArgs& /*args*/)
     {
+        // When we are executing the _SelectNextTab in the TabManagement.cpp, this method
+        // is getting triggered because we set up the default value for that CommandPalette
+        // with an empty string. Therefore, to avoid the reset of the index when executing
+        // the Next/Prev tab command, we are skipping this execution.
+        // Check issue https://github.com/microsoft/terminal/issues/11146
+        if (_currentMode == CommandPaletteMode::TabSwitchMode)
+        {
+            return;
+        }
+
         if (_currentMode == CommandPaletteMode::CommandlineMode)
         {
             _evaluatePrefix();


Test patch: