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.

Sign up for GitHub

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

Jump to bottom

Introduce duplicate tab menu #9388

Merged
2 commits merged into from
Mar 8, 2021
Merged

Introduce duplicate tab menu #9388

2 commits merged into from
Mar 8, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Mar 5, 2021

PR Checklist

@ghost ghost added 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. labels Mar 5, 2021
carlos-zamora
carlos-zamora approved these changes Mar 5, 2021
View reviewed changes
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.

Approving with suggestions. Thanks!

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp
// "Duplicate Tab"
Controls::FontIcon duplicateTabSymbol;
duplicateTabSymbol.FontFamily(Media::FontFamily{ L"Segoe MDL2 Assets" });
duplicateTabSymbol.Glyph(L"\xF5ED");
Copy link
Member

Choose a reason for hiding this comment

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

For anyone curious, this is the icon:
Segoe MDL2 Asset Icon

Chromium edge uses this:
Edge Icon

but that's not a Segoe MDL2 Asset.

I wish there was a better icon but I couldn't find one. Seems like Segoe MDL2 Assets don't include any tab-related icons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Didn't find anything less confusing

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Mar 6, 2021
skyline75489
skyline75489 reviewed Mar 8, 2021
View reviewed changes
src/cascadia/TerminalApp/AppActionHandlers.cpp
@@ -39,7 +39,7 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_HandleDuplicateTab(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
_DuplicateTabViewItem();
_DuplicateFocusedTab();
Copy link
Collaborator

Choose a reason for hiding this comment

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

A stupid question: if a tab is not focused, can it be duplicated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda want see a image of gif demonstrating this feature. There's no image so I can only guess how it will behave based on the method name 😅

Copy link
Contributor Author

@Don-Vito Don-Vito Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here I just renamed the method for duplicating the current tab (in the TerminalPage terminology, current tab is called focused tab, since it is returned by GetFocusedTabIndex).

The addition in the rest of the code is to allow duplication of the tab which is not necessarily the current tab, by clicking on "Duplicate Tab" in the TabView's context menu.

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 8, 2021

@skyline75489 - here is an image! 😄
DuplicateTabCM

zadjii-msft
zadjii-msft approved these changes Mar 8, 2021
View reviewed changes
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.

Yea I'm cool with this. Maybe in the future, we should add a index property to the duplicateTab action, so we can use the ShortcutActionDispatch to handle this, but that's something to deal with in the far future.

Thanks!

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 8, 2021
@ghost
Copy link

ghost commented Mar 8, 2021

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 629c06d into microsoft:main Mar 8, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 8, 2021

Yea I'm cool with this. Maybe in the future, we should add a index property to the duplicateTab action, so we can use the ShortcutActionDispatch to handle this, but that's something to deal with in the far future.

Thanks!

Hey. Actually I didn't work to add index (it could be easier to implement) 😄
The reason is a race, if there is some tab closed in meanwhile you might duplicate the wrong tab.

@zadjii-msft zadjii-msft mentioned this pull request Apr 9, 2021
Closed
@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skyline75489 skyline75489 skyline75489 left review comments

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@DHowett DHowett Awaiting requested review from DHowett

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
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-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Don-Vito @skyline75489 @carlos-zamora @zadjii-msft