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

Advanced Tab Switcher #6732

Merged
64 commits merged into from
Aug 11, 2020
Merged

Advanced Tab Switcher #6732

64 commits merged into from
Aug 11, 2020

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Jun 30, 2020

TabSwitchingv2

Summary of the Pull Request

This PR adds the Advanced Tab Switcher (ATS) to Terminal. It'll work
similarly to VSCode's tab switcher. Because this implementation rides
off a lot of the Command Palette's XAML code, it'll look just like the
Command Palette, and also have support for tab title search.

References

#3753 - ATS Spec

Closes #1502

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UX Issues

Downloaded the branch and got to play around with it for a bit. Found a few big UX issues. After they get fixed/resolved, I'll do another sweep and test out accessibility.

Command Palette Inaccessible

After opening the Tab Switcher, I can now only open the Tab Switcher. When I press the Command Palette keybinding, the Tab Switcher opens instead.

Going Backwards on the Tab Switcher

I bound tabSwitcher to ctrl+tab. Pressing tab gets me the next item in the list. But I'd expect pressing ctrl+shift+tab to go to the previous item.

"Search" box

When we're in anchor mode, it feels a bit weird to have a text box appear, but we can't type into it. I'd bet screen readers would make this extra confusing too because it would read the text box, that you can't type into.

Tab Switcher Results Order

I can't figure out the of the results (I think I got into a bad state somehow). I'd expect them to be in order by index though.

Rearranging Tabs

I created a new tab, then dragged it to index 2 (from 4). I somehow ended up with this:
duplicate entry in order

src/cascadia/TerminalApp/ActionAndArgs.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ActionArgs.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ActionArgs.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.cpp Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 30, 2020
@komawoyo
Copy link

komawoyo commented Jul 5, 2020

Just an idea. Would it be favorable to also include the icons next to the names in the drop down? Maybe a before and after to compare.

@ghost ghost added Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Jul 8, 2020
@leonMSFT
Copy link
Contributor Author

leonMSFT commented Jul 9, 2020

image

Maybe there needs to be a bit more spacing between the "Icon" column and the "Name" column?

@zadjii-msft
Copy link
Member

Design feedback: (full review in progress)

I fricken love that you got the icons figured out. Now we can follow up by letting commands specify the icon too! (not in this PR for sure)

I agree that they maybe need a tad bit more spacing.

I'm not sure I love the labels for Index:0 etc.

Maybe change the Type a command name... string depending on which mode we're in?

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really happy with how smooth this seems. I've got nits throughout, so I'm blocking just so GH won't lose my progress, but I'm really looking forward to this.

src/cascadia/TerminalApp/ActionArgs.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ActionArgs.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Command.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/CommandPalette.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 9, 2020
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 7, 2020
@DHowett DHowett removed their assignment Aug 7, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 10, 2020
@zadjii-msft
Copy link
Member

its-happening

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 11, 2020
@ghost
Copy link

ghost commented Aug 11, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit b07c1e4 into master Aug 11, 2020
@ghost ghost deleted the dev/lelian/ats-take2 branch August 11, 2020 14:03
ghost pushed a commit that referenced this pull request Aug 21, 2020
## Summary of the Pull Request

![cmdpal-icons](https://user-images.githubusercontent.com/18356694/90916410-97dada00-e3a6-11ea-9fb0-755938a68a05.gif)

Adds support for setting a command's `icon`. This supports a couple different scenarios:
* setting a path to an image
* on `"iterateOn": "profiles"` commands, setting the icon to `${profile.icon}` (to use the profile's icon)
* setting the icon to a symbol from [Segoe MDL2 Assets](https://docs.microsoft.com/en-us/windows/uwp/design/style/segoe-ui-symbol-font)
* setting the icon to an emoji
* setting the icon to a character (what is an emoji other than a character, after all?)

## References
* Big s/o to @leonMSFT in #6732, who really did all the hard work here.

## PR Checklist
* [x] Closes #6644 
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Importantly, the creation of these icons must occur on the UI thread. That's why it's done in a "load the path from json", then "get the actual IconSource" structure.

## Validation Steps Performed
see the gif
@ghost
Copy link

ghost commented Aug 26, 2020

🎉Windows Terminal Preview v1.3.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

@Dr-WaSaBi
Copy link

Love all the new stuff in 1.3. Great job

Russ

@simonratner
Copy link

simonratner commented Sep 4, 2020

We'd love feedback on why the new tab switcher didn't work for you!

Not sure where this feedback should go, this seems like a reasonable place. I found it distracting to pop up on ctrl+tab every time, so I turned it off.

I do find the list of tabs to be valuable, but would prefer it to be accessible with a separate key (I guess that's the tabSearch action, which I have bound to ctrl+`).

@pete-debiase
Copy link

Strongly agree with @simonratner on the Advanced Tab Switcher - when I press ctrl+tab or ctrl+shift+tab, I just want to go to the next/previous tab, not get a modal popup. The tabSearch action does seem to be a better application of this functionality in terms of having a way to efficiently navigate through large numbers of open tabs.

Having said that, I thought the implementation of Advanced Tab Switcher was very clean. And since you exposed the useTabSwitcher setting, we can all just enable/disable the feature as desired.

You guys rock. Please keep making Windows Terminal great!

ghost pushed a commit that referenced this pull request Oct 7, 2020
## Summary of the Pull Request

![preview-ats-000](https://user-images.githubusercontent.com/18356694/94801728-18302a00-03ac-11eb-851d-760b92ebb46f.gif)

This PR enables the ATS to display the active tab as the user navigates the tab switcher. We do this by dispatching the tab switch actions as the user navigates the menu, and manually _not_ focusing the new tab when the tab switcher is open. 

## References

* #6732 - original tab switcher PR
* #6689 - That's a more involved, generic version of this, but this PR will be enough to stop most of the complaints hopefully


## PR Checklist
* [x] Closes #7409
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

Opened tabs, tabbed through the menu, verified that it did what I'd expect
DHowett pushed a commit that referenced this pull request Oct 19, 2020
## Summary of the Pull Request

![preview-ats-000](https://user-images.githubusercontent.com/18356694/94801728-18302a00-03ac-11eb-851d-760b92ebb46f.gif)

This PR enables the ATS to display the active tab as the user navigates the tab switcher. We do this by dispatching the tab switch actions as the user navigates the menu, and manually _not_ focusing the new tab when the tab switcher is open.

## References

* #6732 - original tab switcher PR
* #6689 - That's a more involved, generic version of this, but this PR will be enough to stop most of the complaints hopefully

## PR Checklist
* [x] Closes #7409
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

Opened tabs, tabbed through the menu, verified that it did what I'd expect

(cherry picked from commit 22887d7)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: An advanced tab switcher
10 participants