Allow the Tab Switcher to initialize going backwards #9998

@leonMSFT commented on GitHub (Aug 11, 2020):

Just to clarify, do you mean that the user would have this line in their settings:

{"command": {"action":"tabSwitcher", "anchorKey":"ctrl"}, "keys":"ctrl+tab"}

and we would magically in the back also allow ctrl+shift+tab to open the tab switcher going backwards (unless there's an override like in your example) ?

I definitely agree that using shift and expecting it to go backwards while you have the above binding set is kind of intuitive, but I feel safer and more comfortable with making the user explicitly tell us that ctrl+shift+tab opens the tab switcher. Although, I'm still not decided on whether or not they should also tell us that ctrl+shift+tab goes backwards and ctrl+tab goes forwards.

We also currently have nextTab and prevTab bound to ctrl+tab and ctrl+shift+tab respectively. If the user wants to switch over to using ATS, they'd have to at the very least unbind both of those commands to free up the keychords.

@leonMSFT commented on GitHub (Aug 11, 2020): Just to clarify, do you mean that the user would have this line in their settings: ```json {"command": {"action":"tabSwitcher", "anchorKey":"ctrl"}, "keys":"ctrl+tab"} ``` and we would magically in the back also allow <kbd>ctrl+shift+tab</kbd> to open the tab switcher going backwards (unless there's an override like in your example) ? I definitely agree that using <kbd>shift</kbd> and expecting it to go backwards while you have the above binding set is kind of intuitive, but I feel safer and more comfortable with making the user explicitly tell us that <kbd>ctrl+shift+tab</kbd> opens the tab switcher. Although, I'm still not decided on whether or not they should _also_ tell us that <kbd>ctrl+shift+tab</kbd> goes backwards and <kbd>ctrl+tab</kbd> goes forwards. We also currently have `nextTab` and `prevTab` bound to <kbd>ctrl+tab</kbd> and <kbd>ctrl+shift+tab</kbd> respectively. If the user wants to switch over to using ATS, they'd have to at the very least unbind both of those commands to free up the keychords.

claunia commented

@carlos-zamora commented on GitHub (Aug 11, 2020):

Yeah.

{"command": {"action":"tabSwitcher", "anchorKey":"ctrl"}, "keys":"ctrl+tab"}

My concern is that if we go the route of needing a keybinding arg or new keybinding for "tab switcher going backwards", what happens if you do not have ctrl+shift+tab defined as a keybinding. Like, we're kinda forcing users to define two keybindings for one concept.

Ugh. But then you can also say the opposite. What happens if somebody does this:

{ "command": null, "keys": "ctrl+shift+tab" }

That should probably disable shift for tab switcher, but it won't.

Curious to what others think. Maybe just doing what you're saying about nextTab and prevTab would be the right move. :/

@carlos-zamora commented on GitHub (Aug 11, 2020): Yeah. ```json {"command": {"action":"tabSwitcher", "anchorKey":"ctrl"}, "keys":"ctrl+tab"} ``` My concern is that if we go the route of needing a keybinding arg or new keybinding for "tab switcher going backwards", what happens if you _do not_ have `ctrl+shift+tab` defined as a keybinding. Like, we're kinda forcing users to define two keybindings for one concept. Ugh. But then you can also say the opposite. What happens if somebody does this: ```json { "command": null, "keys": "ctrl+shift+tab" } ``` That _should_ probably disable shift for tab switcher, but it won't. Curious to what others think. Maybe just doing what you're saying about `nextTab` and `prevTab` would be the right move. :/

claunia commented

@DHowett commented on GitHub (Aug 11, 2020):

We already have two bindings for the one thing (next/prev)

@DHowett commented on GitHub (Aug 11, 2020): We _already_ have two bindings for the one thing (next/prev)

claunia commented

@leonMSFT commented on GitHub (Aug 11, 2020):

I think if we look at the anchored ATS keybinding not just as an "open tab switcher" keybinding, but also as a prevTab/nextTab keybinding (since you'd achieve the same effect of the current prevTab/nextTab when tapping the keybinding, it'll just super quickly open/close the ATS as well), it might make more sense to force the users to create two keybindings: one for openATS + prevTab, and one for openATS + nextTab. It'll basically replace the existing nextTab/prevTab!

In that case, does initialDirection make sense as an arg, and maybe the args should be named prevTab and nextTab, with default being nextTab if initialDirection is omitted? 🤔

@leonMSFT commented on GitHub (Aug 11, 2020): I think if we look at the anchored ATS keybinding not just as an "open tab switcher" keybinding, but _also_ as a prevTab/nextTab keybinding (since you'd achieve the same effect of the current prevTab/nextTab when tapping the keybinding, it'll just super quickly open/close the ATS as well), it might make more sense to force the users to create two keybindings: one for openATS + prevTab, and one for openATS + nextTab. It'll basically replace the existing nextTab/prevTab! In that case, does `initialDirection` make sense as an arg, and maybe the args should be named `prevTab` and `nextTab`, with default being `nextTab` if `initialDirection` is omitted? 🤔

claunia commented

@leonMSFT commented on GitHub (Aug 12, 2020):

Alright here's a totally different approach that might simplify this a lot. How about we simply override nextTab/prevTab (nT/pT) to use the anchored ATS, and turn unanchored ATS into tabSearch?

Chances are, users aren't going to be setting both nT/pT along with anchored ATS. They would be using one or the other. In the interest of letting users choose which experience they want, we could provide a useNewTabSwitcherExperience flag to swap the behavior of nT/pT. We could also get rid of the flag once this is out of preview and force users to the new experience. If the flag is true, nT/pT will open what is right now considered anchored ATS with the next/prev tab focused. If the flag is false, nT/pT will do what it does currently. Then we would end up having a tabSearch keybinding, which opens unanchored ATS with its search bar and all.

A couple of issues come to mind if ATS takes over nextTab/prevTab though:

Without an anchorKey argument, we'd be forced to hardcode ctrl as the anchor key, and if people decide to customize nT/pT in such a way where ctrl isn't included in their keychord, they're gonna have a bad time. I'm not sure if it's possible to detect modifiers in a given keychord so that we can just auto assign an anchor key? We could also just not allow nT/pT to be customized?
Invoking nT/pT in the cmdpal would be kinda weird since right now, anchored mode relies on the user releasing the anchor key in order to select the tab.

Thoughts?

@leonMSFT commented on GitHub (Aug 12, 2020): Alright here's a totally different approach that might simplify this a lot. How about we simply override `nextTab/prevTab` (nT/pT) to use the anchored ATS, and turn unanchored ATS into `tabSearch`? Chances are, users aren't going to be setting both nT/pT along with anchored ATS. They would be using one or the other. In the interest of letting users choose which experience they want, we _could_ provide a `useNewTabSwitcherExperience` flag to swap the behavior of nT/pT. We could also get rid of the flag once this is out of preview and force users to the new experience. If the flag is true, nT/pT will open what is right now considered anchored ATS with the next/prev tab focused. If the flag is false, nT/pT will do what it does currently. Then we would end up having a `tabSearch` keybinding, which opens unanchored ATS with its search bar and all. A couple of issues come to mind if ATS takes over nextTab/prevTab though: - Without an `anchorKey` argument, we'd be forced to hardcode <kbd>ctrl</kbd> as the anchor key, and if people decide to customize nT/pT in such a way where <kbd>ctrl</kbd> isn't included in their keychord, they're gonna have a bad time. I'm not sure if it's possible to detect modifiers in a given keychord so that we can just auto assign an anchor key? We could also just not allow nT/pT to be customized? - Invoking nT/pT in the cmdpal would be kinda weird since right now, anchored mode relies on the user _releasing_ the anchor key in order to select the tab. Thoughts?

claunia commented

@DHowett commented on GitHub (Aug 13, 2020):

The thing I like about nT/pT being "move forward and backward in an anchored tab switcher" is that it makes our key bindings more aware of the things that are happening in the app. "Next tab" has a different meaning when the anchored switcher is open -- it just means "move to the next entry or wrap."

My argument in favor is: I expect that users aren't going to want to bind next/prev tab and "open tabswitcher with next/prev selected" simultaneously. We have to make a call about what our experience is going to be here.

@DHowett commented on GitHub (Aug 13, 2020): The thing I like about nT/pT being "move forward and backward in an anchored tab switcher" is that it makes our key bindings more aware of the things that are happening in the app. "Next tab" has a different meaning when the anchored switcher is open -- it just means "move to the next entry or wrap." My argument in favor is: I expect that users aren't going to want to bind next/prev tab _and_ "open tabswitcher with next/prev selected" simultaneously. We have to make a call about what our experience is going to be here.

claunia commented

@carlos-zamora commented on GitHub (Aug 13, 2020):

nT/pT already wraps.

I like the idea of the unanchored tab switcher being separated out. I feel like tabSearch could be a mode in the command palette, and we just allocate a prefix for it (similar to commandline mode and action mode).

If we have tab switcher only operate in anchored mode, we also run into the following problem:

what if I bind tabSwitcher to f1?

Maybe if no modifier is provided or we hit a weird situation, we should just fall back to tabSwitcher being unanchored? Otherwise it makes no sense. (this would be a solution to Leon's 2 points above too)

@carlos-zamora commented on GitHub (Aug 13, 2020): nT/pT already wraps. I like the idea of the unanchored tab switcher being separated out. I feel like `tabSearch` could be a mode in the command palette, and we just allocate a prefix for it (similar to commandline mode and action mode). If we have tab switcher only operate in anchored mode, we also run into the following problem: - what if I bind `tabSwitcher` to <kbd>f1</kbd>? Maybe _if_ no modifier is provided or we hit a weird situation, we should just fall back to tabSwitcher being unanchored? Otherwise it makes no sense. (this would be a solution to Leon's 2 points above too)

claunia commented

@carlos-zamora commented on GitHub (Aug 13, 2020):

I'm also curious what @cinnamon-msft 's thoughts are as our UX person

@carlos-zamora commented on GitHub (Aug 13, 2020): I'm also curious what @cinnamon-msft 's thoughts are as our UX person

claunia commented

@DHowett commented on GitHub (Aug 13, 2020):

it just means "move to the next entry or wrap."

nT/pT already wraps.

My point was not that it would suddenly start wrapping, my point was that it would suddenly start "doing the right thing" when there was ephemeral UI (the switcher overlay.)

@DHowett commented on GitHub (Aug 13, 2020): > it just means "move to the next entry or wrap." > nT/pT already wraps. My point was not that it would suddenly start wrapping, my point was that it would suddenly start "doing the right thing" when there was ephemeral UI (the switcher overlay.)

claunia commented

@DHowett commented on GitHub (Aug 13, 2020):

Invoking nT/pT in the cmdpal would be kinda weird since right now, anchored mode relies on the user releasing the anchor key in order to select the tab.

This would be fine. If nT/pT are activated by the keyboard and the global useNewTabSwitchingExperience (whatever) is set, it does the switcher. If you manually invoke a nT/pT command in any other way, it just does the thing.

Just like Carlos's Mark mode - things act different depending on the mode.

@DHowett commented on GitHub (Aug 13, 2020): > Invoking nT/pT in the cmdpal would be kinda weird since right now, anchored mode relies on the user releasing the anchor key in order to select the tab. This would be fine. If nT/pT are activated by the keyboard and the global `useNewTabSwitchingExperience` (whatever) is set, it does the switcher. If you manually invoke a nT/pT command _in any other way_, it just does the thing. Just like Carlos's Mark mode - things act different depending on the mode.

claunia commented

@cinnamon-msft commented on GitHub (Aug 13, 2020):

I have a few thoughts off the top of my head:

For unanchored mode, the current behavior opens with the focused tab selected, so I'm not sure this issue would affect that. Additionally, is there a benefit to having both the command palette and the unanchored tab switcher? These seem fairly redundant in my opinion.

For anchored mode, I would assume the behavior for pT/nT should align with the tab switcher. It seems funny to let both the tab switcher previous/next and tab focus previous/next be customizable. I'm wondering if we should just have a global "use the tab switcher" setting and then use the pT/nT values for navigation. This may provide a technical difficulty for finding the anchor key though.

@cinnamon-msft commented on GitHub (Aug 13, 2020): I have a few thoughts off the top of my head: For unanchored mode, the current behavior opens with the focused tab selected, so I'm not sure this issue would affect that. Additionally, is there a benefit to having *both* the command palette and the unanchored tab switcher? These seem fairly redundant in my opinion. For anchored mode, I would assume the behavior for pT/nT should align with the tab switcher. It seems funny to let both the tab switcher previous/next and tab focus previous/next be customizable. I'm wondering if we should just have a global "use the tab switcher" setting and then use the pT/nT values for navigation. This may provide a technical difficulty for finding the anchor key though.

claunia commented

@DHowett commented on GitHub (Aug 13, 2020):

I'm wondering if we should just have a global "use the tab switcher" setting and then use the pT/nT values for navigation.

YES! Kayla independently suggested the same thing as Leon in https://github.com/microsoft/terminal/issues/7178#issuecomment-673148673

🎲

@DHowett commented on GitHub (Aug 13, 2020): >I'm wondering if we should just have a global "use the tab switcher" setting and then use the pT/nT values for navigation. YES! Kayla independently suggested the same thing as Leon in https://github.com/microsoft/terminal/issues/7178#issuecomment-673148673 🎲

claunia commented