MRU useTabSwitcher prevents switching to tabs in left/right order #12796

@Don-Vito commented on GitHub (Mar 9, 2021):

@zadjii-msft - I can add support for:

{ "action": "nextTab", "tabSwitcher": "inOrder" },
{ "action": "prevTab", "tabSwitcher": "MRU" }

WDYT?
Thought I am not sure what they should do if the tabswitcher is disabled.

@Don-Vito commented on GitHub (Mar 9, 2021): @zadjii-msft - I can add support for: ``` { "action": "nextTab", "tabSwitcher": "inOrder" }, { "action": "prevTab", "tabSwitcher": "MRU" } ``` WDYT? Thought I am not sure what they should do if the tabswitcher is disabled.

claunia commented

@johnburnett commented on GitHub (Mar 10, 2021):

Hmm, honestly feels less than ideal? Requiring the use of the tab switcher feels really odd from the outside, especially for those not wanting it?

@johnburnett commented on GitHub (Mar 10, 2021): Hmm, honestly feels less than ideal? Requiring the use of the tab switcher feels really odd from the outside, especially for those not wanting it?

claunia commented

@Don-Vito commented on GitHub (Mar 10, 2021):

Hmm, honestly feels less than ideal? Requiring the use of the tab switcher feels really odd from the outside, especially for those not wanting it?

Guess I can make it:

{ "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false },

If not provided, use global settings.
WDYT?

@Don-Vito commented on GitHub (Mar 10, 2021): > > > Hmm, honestly feels less than ideal? Requiring the use of the tab switcher feels really odd from the outside, especially for those not wanting it? Guess I can make it: ``` { "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false }, ``` If not provided, use global settings. WDYT?

claunia commented

@zadjii-msft commented on GitHub (Mar 10, 2021):

{ "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false },

If not provided, use global settings.

That seems reasonable to me. Currently, {"action": "nextTab", "tabSwitchOrder": "mru", "useTabSwitcher": false} wouldn't work - we'd need to solve #9390 to make that work. But presuming that #9390 is eventually solvable, then this design is ready for that scenario.

My only question is what do we do when someone sets {"mru", false} before we fix #9390? Silently falling back to {"inOrder", false} feels wrong, {"mru", true} I suppose is a bit better. Maybe we could throw a warning when we're parsing the nextTab args that the {"mru", false} combo doesn't work, and we'll fall back to {"mru", true}? Then at least the user knows what's up.

@zadjii-msft commented on GitHub (Mar 10, 2021): > ``` > { "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false }, > ``` > > > If not provided, use global settings. That seems reasonable to me. Currently, `{"action": "nextTab", "tabSwitchOrder": "mru", "useTabSwitcher": false}` wouldn't work - we'd need to solve #9390 to make that work. But presuming that #9390 is _eventually_ solvable, then this design is ready for that scenario. My only question is what do we do when someone sets `{"mru", false}` _before_ we fix #9390? Silently falling back to `{"inOrder", false}` feels wrong, `{"mru", true}` I suppose is a bit better. Maybe we could throw a warning when we're parsing the `nextTab` args that the `{"mru", false}` combo doesn't work, and we'll fall back to `{"mru", true}`? Then at least the user knows what's up.

claunia commented

@Don-Vito commented on GitHub (Mar 10, 2021):

@zadjii-msft - as suggested in #9390, let's unify these tickets, they come together.

@Don-Vito commented on GitHub (Mar 10, 2021): @zadjii-msft - as suggested in #9390, let's unify these tickets, they come together.

claunia commented

@johnburnett commented on GitHub (Mar 10, 2021):

I'm not sure which ticket to respond to, so I'll stick with this one for now? Doing this:

{ "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false },

...doesn't look horrible, but including "useTabSwitcher" in this action definition still feels like combining two things that shouldn't be combined. I'm guessing this is because the existing "tabSwitcherMode" exists, and you are trying to override it?

Separately, I'd lean towards renaming "tabSwitchOrder" to something like "direction" or "order" instead (it took me a beat to recognize "tabSwitchOrder" had nothing to do with the tab switcher UI).

@johnburnett commented on GitHub (Mar 10, 2021): I'm not sure which ticket to respond to, so I'll stick with this one for now? Doing this: > `{ "action": "nextTab", "tabSwitchOrder": "inOrder/MRU", "useTabSwitcher": true/false },` ...doesn't look horrible, but including "useTabSwitcher" in this action definition still feels like combining two things that shouldn't be combined. I'm guessing this is because the existing "tabSwitcherMode" exists, and you are trying to override it? Separately, I'd lean towards renaming "tabSwitchOrder" to something like "direction" or "order" instead (it took me a beat to recognize "tabSwitchOrder" had nothing to do with the tab switcher UI).

claunia commented

@Don-Vito commented on GitHub (Mar 15, 2021):

After some hesitation, as I am not sure when I get to #9390, I decided to use the following format:

{ "command": { "action": "prevTab", "tabSwitcherMode": "inOrder" }, "keys": "ctrl+f1"}
{ "command": { "action": "nextTab", "tabSwitcherMode": "inOrder" }, "keys": "ctrl+f2"}

or

{"command": { "action": "prevTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+f1"}
{ "command": { "action": "nextTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+f2"}

if you don't want tab switcher to show up.

It is actually aligned with the global setting. If we decide to add MRU mode with no tab switcher, we can simply add the new value to the enum (aka "disabledMRU"), and this will work for both commands and the settings.

@Don-Vito commented on GitHub (Mar 15, 2021): After some hesitation, as I am not sure when I get to #9390, I decided to use the following format: ``` { "command": { "action": "prevTab", "tabSwitcherMode": "inOrder" }, "keys": "ctrl+f1"} { "command": { "action": "nextTab", "tabSwitcherMode": "inOrder" }, "keys": "ctrl+f2"} ``` or ``` {"command": { "action": "prevTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+f1"} { "command": { "action": "nextTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+f2"} ``` if you don't want tab switcher to show up. It is actually aligned with the global setting. If we decide to add MRU mode with no tab switcher, we can simply add the new value to the enum (aka "disabledMRU"), and this will work for both commands and the settings.

claunia commented

@johnburnett commented on GitHub (Mar 15, 2021):

tbh this setup feels... bizarre. But I guess that's the price of backwards
compatibility? The "if we add MRU support without tab switcher" is also a
slight cause for concern. Also wondering about the kinda-not-standard key
bindings as well?

@johnburnett commented on GitHub (Mar 15, 2021): tbh this setup feels... bizarre. But I guess that's the price of backwards compatibility? The "if we add MRU support without tab switcher" is also a slight cause for concern. Also wondering about the kinda-not-standard key bindings as well?

claunia commented

@zadjii-msft commented on GitHub (Mar 15, 2021):

Also wondering about the kinda-not-standard key bindings as well?

We're not going to be binding this by default - those keys listed are mostly as examples ☺️

@zadjii-msft commented on GitHub (Mar 15, 2021): > Also wondering about the kinda-not-standard key bindings as well? We're not going to be binding this by default - those keys listed are mostly as examples ☺️

claunia commented

@Don-Vito commented on GitHub (Mar 15, 2021):

tbh this setup feels... bizarre. But I guess that's the price of backwards compatibility? The "if we add MRU support without tab switcher" is also a slight cause for concern. Also wondering about the kinda-not-standard key bindings as well?

Yes.. this is a price of backward-compatibility between the settings and the commands parameters. I tried to do what we discussed earlier and it was confusing as hell 😊 I mean in the global settings you would have an enum, and here you will have two flags, and the entire resolution logic (aka what happens if default is TabSwitcherMode=Disabled, and here useTabSwitcher=True) would be very non-intuitive.

With this solution it is relatively simple: if you have enum defined in param - it will be used, default otherwise.

Regarding the samples, as Mike said, we are not going to bind them, I put the ones I was using for testing, as an example.

@Don-Vito commented on GitHub (Mar 15, 2021): > > tbh this setup feels... bizarre. But I guess that's the price of backwards compatibility? The "if we add MRU support without tab switcher" is also a slight cause for concern. Also wondering about the kinda-not-standard key bindings as well? Yes.. this is a price of backward-compatibility between the settings and the commands parameters. I tried to do what we discussed earlier and it was confusing as hell :blush: I mean in the global settings you would have an enum, and here you will have two flags, and the entire resolution logic (aka what happens if default is TabSwitcherMode=Disabled, and here useTabSwitcher=True) would be very non-intuitive. With this solution it is relatively simple: if you have enum defined in param - it will be used, default otherwise. Regarding the samples, as Mike said, we are not going to bind them, I put the ones I was using for testing, as an example.

claunia commented

@ghost commented on GitHub (Apr 14, 2021):

:tada:This issue was addressed in #9507, which has now been successfully released as Windows Terminal Preview v1.8.1032.0.🎉

Handy links:

@ghost commented on GitHub (Apr 14, 2021): :tada:This issue was addressed in #9507, which has now been successfully released as `Windows Terminal Preview v1.8.1032.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.8.1032.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)

claunia commented

@johnburnett commented on GitHub (May 25, 2021):

This doesn't seem to be working in 1.8.1444.0. See below for repro steps, but it doesn't seem possible to make a config that does this:

"ctrl+pgdn" always goes to the right (with wrapping)
"ctrl+pgup" always goes to the left (with wrapping)
"ctrl+tab" always goes to the previously selected tab in MRU order
"ctrl+shift+tab" always goes to the next selected tab in MRU order

Steps:

Put this in your config:

{
    "useTabSwitcher": true,
    "actions":
    [
        { "command": "nextTab", "tabSwitcherMode": "inOrder", "keys": "ctrl+pgdn" },
        { "command": "prevTab", "tabSwitcherMode": "inOrder", "keys": "ctrl+pgup" },
        { "command": "nextTab", "tabSwitcherMode": "mru", "keys": "ctrl+tab" },
        { "command": "prevTab", "tabSwitcherMode": "mru", "keys": "ctrl+shift+tab" }
    ]
}

Open a terminal
Create a tab and rename it "A"
Create a tab and rename it "B"
Create a tab and rename it "C"
Hit "ctrl+pgdn" but don't release "ctrl"

I'd expect to have switched to the next right-most tab from "C" (which is "A" at this point), but instead it switches to "B"
Looking at the tab switcher list (because "ctrl" is still being held), you can see why: Terminal thinks the tab order is "C", "B", "A" for some reason.

Release "ctrl"
Hit "ctrl+pgup" and don't release "ctrl" again

I'd expect to go to "A" (and you do!), but note the tab switcher order is listed as "B", "C", "A"

Release "ctrl"
Hit "ctrl+pgdn" and before releasing "ctrl", note the tab switcher order is "A", "B", "C"

You should be on tab "B" at this point with no tab switcher.

Hit "ctrl+pgdn" and release "ctrl"

You go to tab "A" instead of "C". Repeating step 10 makes you bounce back and forth between "A" and "B" forever.

Given how all this is currently implemented it seems like the only way to hit the original request would be to either introduce an "adjacent" or "leftright" mode for "tabSwitcherMode", or to consider the current "inOrder" behavior a bug, and change it.

@johnburnett commented on GitHub (May 25, 2021): This doesn't seem to be working in 1.8.1444.0. See below for repro steps, but it doesn't seem possible to make a config that does this: - "ctrl+pgdn" always goes to the right (with wrapping) - "ctrl+pgup" always goes to the left (with wrapping) - "ctrl+tab" always goes to the previously selected tab in MRU order - "ctrl+shift+tab" always goes to the next selected tab in MRU order Steps: Put this in your config: ``` { "useTabSwitcher": true, "actions": [ { "command": "nextTab", "tabSwitcherMode": "inOrder", "keys": "ctrl+pgdn" }, { "command": "prevTab", "tabSwitcherMode": "inOrder", "keys": "ctrl+pgup" }, { "command": "nextTab", "tabSwitcherMode": "mru", "keys": "ctrl+tab" }, { "command": "prevTab", "tabSwitcherMode": "mru", "keys": "ctrl+shift+tab" } ] } ``` 1. Open a terminal 2. Create a tab and rename it "A" 3. Create a tab and rename it "B" 4. Create a tab and rename it "C" 5. Hit "ctrl+pgdn" but don't release "ctrl" I'd expect to have switched to the next right-most tab from "C" (which is "A" at this point), but instead it switches to "B" Looking at the tab switcher list (because "ctrl" is still being held), you can see why: Terminal thinks the tab order is "C", "B", "A" for some reason. 6. Release "ctrl" 7. Hit "ctrl+pgup" and don't release "ctrl" again I'd expect to go to "A" (and you do!), but note the tab switcher order is listed as "B", "C", "A" 8. Release "ctrl" 9. Hit "ctrl+pgdn" and before releasing "ctrl", note the tab switcher order is "A", "B", "C" You should be on tab "B" at this point with no tab switcher. 10. Hit "ctrl+pgdn" and release "ctrl" You go to tab "A" instead of "C". Repeating step 10 makes you bounce back and forth between "A" and "B" forever. Given how all this is currently implemented it seems like the only way to hit the original request would be to either introduce an "adjacent" or "leftright" mode for "tabSwitcherMode", or to consider the current "inOrder" behavior a bug, and change it.

claunia commented

@DHowett commented on GitHub (May 25, 2021):

Huh, you're totally right. No matter what mode is set in the action, it still uses the switcher... even if it's disabled. That's way wrong.

@DHowett commented on GitHub (May 25, 2021): _Huh_, you're totally right. No matter what mode is set in the action, it still uses the switcher... even if it's `disabled`. That's way wrong.

claunia commented

@DHowett commented on GitHub (May 25, 2021):

Wait a moment...

We may have fallen prey to legacy command support.

Take two of these ("and call me in the morning"):

        { "command": { "action": "nextTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+pgdn" },
        { "command": { "action": "prevTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+pgup" },

EDIT: Substitute inOrder if you'd like. The key is the nested object to contain the "arguments".

@DHowett commented on GitHub (May 25, 2021): Wait a moment... We may have fallen prey to legacy command support. Take two of these ("and call me in the morning"): ```json { "command": { "action": "nextTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+pgdn" }, { "command": { "action": "prevTab", "tabSwitcherMode": "disabled" }, "keys": "ctrl+pgup" }, ``` EDIT: Substitute `inOrder` if you'd like. The key is the nested object to contain the "arguments".

claunia commented