search

microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.71k stars 8.33k forks source link

Allow the Tab Switcher to initialize going backwards #7178

Closed leonMSFT closed 4 years ago

leonMSFT commented 4 years ago

6732 implements the Tab Switcher in such a way that when in anchor mode, the ATS will always open up with the currently focusedTab + 1 selected. There's currently no way for a user to say if they wanted the keybinding to open the ATS with the currently focusedTab - 1 selected.

So, if the user bound ctrl+tab to tabSwitcher, the user would probably want to switch to a previous tab instead by pressing ctrl+shift+tab. Assuming both keychords are bound to tabSwitcher, they'll both open the ATS with focusedTab + 1 selected.

The ATS PR was going to implement it in a way where we'd add an initialDirection argument to let us know which direction the ats would open in, but it rightfully warranted some more discussion, So, to unblock #6732, I've pulled this out into its own follow up issue here.

Another option in the interest of avoiding passing another argument could be to detect if shift is held down at the AppActionHandlers.cpp level and determine + pass the initial delta to the tab switcher for initialization?

miniksa commented 4 years ago

After talking with @leonMSFT, I'm not sure what the alternative implementation here is to just using an optional initialDirection argument that defaults to forward. We shouldn't automatically capture the Shift modifier unless the user has specified it. No one likes "magic" like that.

It seems like a little bit of overkill using an argument to specify a boolean state, but it seems weirder to me to have two different action verbs like tabSwitcher and tabSwitcherReversed.

carlos-zamora commented 4 years ago

IIRC we're already handling Shift as "move backwards" in the tab switcher. So I think making Shift a magic modifier would be intuitive. I just don't see a use case for a separate keybinding command or arg to represent it.

There's a few scenarios that I think are a bit interesting though...

Overwrite Shift keybinding

{ "command": "tabSwitcher", "keys": "ctrl+tab" },
{ "command": "copy", "keys": "ctrl+shift+tab" }

I think ctrl+shift+tab should do "copy".

Already include Shift in key chord

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

we just interpret that as always go backwards? (not too sure about this scenario :/ )

leonMSFT commented 4 years ago

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.

carlos-zamora commented 4 years ago

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. :/

DHowett commented 4 years ago

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

leonMSFT commented 4 years ago

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 4 years ago

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:

Thoughts?

DHowett commented 4 years ago

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.

carlos-zamora commented 4 years ago

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:

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 4 years ago

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

DHowett commented 4 years ago

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 4 years ago

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.

cinnamon-msft commented 4 years ago

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.

DHowett commented 4 years ago

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

🎲

leonMSFT commented 4 years ago

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."

Oh this got me thinking - currently the ATS is just hardcoded to detect tab/shift+tab/down/up keydowns to navigate the tab switcher. But it really should also be detecting nT/pT events?keychords? to go next/go back. I'm not sure how that'll work though, since TermControl is the keybinding handler, and I don't (think?) that TermControl will receive the keydowns when ATS is open and focused?

This may provide a technical difficulty for finding the anchor key though.

I was actually thinking - there's maybe no need to define an "anchor key", it really should just be an "anchor keychord". Maybe it should just detect if "no keys are pressed" on a KeyUpHandler (aka you've let go of the last key in your whole nT/pT keychord), and just focus your selected tab then? That way the ATS doesn't need to know any of the keys associated with nT/pT, it'll just be like "ok you're definitely done if you don't have a key anchoring me open". Maybe a less comprehensive and good enough check would just be checking if all modifiers are not pressed?

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)

I honestly think if I go with the whole "detecting no keys pressed" thing above, f1 will simply open/close ATS quickly and focus your next tab. They'll simply need to provide another key if they want it to work "anchored".

leonMSFT commented 4 years ago

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.

By "it just does the thing" do you mean it'll do classic nT/pT, or just whatever the nT/pT handler happens to do (taking into account if the global is set or not)?

DHowett commented 4 years ago

Yeah, that

ghost commented 4 years ago

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

Handy links: