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

Teach Terminal to move tabs with key bindings #8338

Merged
6 commits merged into from
Nov 25, 2020
Merged

Teach Terminal to move tabs with key bindings #8338

6 commits merged into from
Nov 25, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Nov 19, 2020 •
edited
Loading

Summary of the Pull Request

Introduces a new command called moveTab
This command has a single mandatory argument with values of forward and backward

PR Checklist

Detailed Description of the Pull Request / Additional comments

Went for the straightforward solution of moving the tab and the tabViewItem.

Validation Steps Performed

  • Manual testing

@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 Nov 19, 2020
@Don-Vito
Copy link
Contributor Author

@zadjii-msft - please review. I still need to add some tests. At least for parameters parsing.

@github-actions

This comment has been minimized.

Sign in to view
1 similar comment
@github-actions

This comment has been minimized.

Sign in to view
zadjii-msft
zadjii-msft reviewed Nov 20, 2020
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.

Overall, I'm definitely cool with this, just want an answer on the MRU thing before I hit the ✔️

src/cascadia/TerminalSettingsModel/ActionArgs.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
@Don-Vito
Copy link
Contributor Author

@zadjii-msft - may I ask a question about the process? Dometimes when you approve PR you add more reviewers, and sometimes not. Is it related to the fact that you wan to dogfood first - or any other considerations. Feel free to ignore if this is not for public messaging.

@zadjii-msft
Copy link
Member

I'm gonna level with you - there's not usually a rhyme or reason to it. Sometimes I remember the "Needs-Second" label, and other times I don't. Sometimes I'll assign someone if they're a particular expert in the feature area, but with all the command palette work you've been doing, I am usually the feature area expert 😄

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Nov 20, 2020
@Don-Vito
Copy link
Contributor Author

@DHowett - bumping this for the case you want to give me a homework for the long week-end 😊

DHowett
DHowett requested changes Nov 25, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

minor nits and Qs, but I'm obligated to block until we have a documentation PR in hand 😄

src/cascadia/TerminalApp/TerminalPage.cpp Outdated
// - <none>
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex)
{
uint32_t newTabIndex = std::clamp<int32_t>(suggestedNewTabIndex, 0, _tabs.Size() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

qq: should tab move wrap around like tab switch? moving to the right when you're at the end might want to move to the far left?

Copy link
Contributor Author

@Don-Vito Don-Vito Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sincerely speaking I am not sure what will be more useful, thought this will be more aligned with what we have when dragging with a mouse.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionArgs.cpp Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/defaults.json 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 Nov 25, 2020
@Don-Vito Don-Vito mentioned this pull request Nov 25, 2020
Merged
DHowett
DHowett approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Beautiful, as always. Thank you!

@DHowett DHowett changed the title Move tab with key bindings Teach Terminal to move tabs with key bindings Nov 25, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 25, 2020
@ghost
Copy link

ghost commented Nov 25, 2020

Hello @DHowett!

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 f5a016c into microsoft:main Nov 25, 2020
@Don-Vito Don-Vito deleted the 3593-move-tab branch December 3, 2020 17:02
@zadjii-msft zadjii-msft mentioned this pull request Dec 10, 2020
Closed
@cinnamon-msft cinnamon-msft mentioned this pull request Jan 25, 2021
Closed
15 tasks
@DJackman123
Copy link
Contributor

Thank you thank you thank you for adding this!! When running Terminal as administrator (which I always have to do) there has been no way to re-order tabs until now. Sooo glad to see this alternate method that works when elevated!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@DHowett DHowett DHowett approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@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 @zadjii-msft @DJackman123 @DHowett