Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the Tab Switcher to initialize going backwards #7178

Closed
leonMSFT opened this issue Aug 4, 2020 · 18 comments · Fixed by #7321
Closed

Allow the Tab Switcher to initialize going backwards #7178

leonMSFT opened this issue Aug 4, 2020 · 18 comments · Fixed by #7321
Assignees
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@leonMSFT
Copy link
Contributor

leonMSFT commented Aug 4, 2020

#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?

@leonMSFT leonMSFT self-assigned this Aug 4, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 4, 2020
@leonMSFT leonMSFT added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Aug 4, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 4, 2020
@leonMSFT leonMSFT added this to the Terminal v1.3 milestone Aug 4, 2020
@leonMSFT leonMSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 4, 2020
@miniksa
Copy link
Member

miniksa commented Aug 11, 2020

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
Copy link
Member

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
Copy link
Contributor Author

leonMSFT commented 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.

@carlos-zamora
Copy link
Member

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
Copy link
Member

DHowett commented Aug 11, 2020

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

@leonMSFT
Copy link
Contributor Author

leonMSFT commented 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
Copy link
Contributor Author

leonMSFT commented 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?

@DHowett
Copy link
Member

DHowett commented Aug 12, 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.

@carlos-zamora
Copy link
Member

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
Copy link
Member

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

@DHowett
Copy link
Member

DHowett commented 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
Copy link
Member

DHowett commented 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.

@cinnamon-msft
Copy link
Contributor

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
Copy link
Member

DHowett commented 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 #7178 (comment)

🎲

@leonMSFT
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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
Copy link
Member

DHowett commented Aug 13, 2020

Yeah, that

@ghost ghost added the In-PR This issue has a related PR label Aug 17, 2020
@ghost ghost closed this as completed in #7321 Aug 21, 2020
@ghost ghost removed the In-PR This issue has a related PR label Aug 21, 2020
ghost pushed a commit that referenced this issue Aug 21, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This PR splits the anchored and unanchored tab switcher into two. The anchored tab switcher is now baked into `nextTab`/`prevTab`, and the unanchored tab switcher command is just named `tabSearch`. `tabSearch` takes no arguments. To reflect this distinction, `CommandPalette.cpp` now refers to one as `TabSwitchMode` and the other as `TabSearchMode`.

I've added a global setting named `useTabSwitcher` (name up for debate) that makes the Terminal use the anchored tab switcher experience for `nextTab` and `prevTab`. 

I've also given the control the ability to detect <kbd>Alt</kbd> KeyUp events and to dispatch keybinding events. By listening for keybindings, the ATS can react to `nextTab`/`prevTab` invocations for navigation in addition to listening for <kbd>tab</kbd> and the arrow keys.

Closes #7178 
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] CLA signed.
* [x] Documentation updates: MicrosoftDocs/terminal#107
* [x] Schema updated.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 21, 2020
@ghost
Copy link

ghost commented Aug 26, 2020

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

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants