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

Clamp the focusTab action to the number of available tabs #10651

Merged
7 commits merged into from
Jul 22, 2021
Merged

Clamp the focusTab action to the number of available tabs #10651

7 commits merged into from
Jul 22, 2021

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

When we perform a focusTab action, we currently do nothing if the parameter was greater than the number of tabs. This PR changes that behavior. Now, focus-tab -t 999999 will always focus the last tab, instead of silently doing nothing.

PR Checklist

Validation Steps Performed

  • ran tests
  • validated commandline manually

@ghost ghost added Area-Commandline wt.exe's commandline arguments Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jul 13, 2021
@DHowett
Copy link
Member

DHowett commented Jul 13, 2021

Hmmmmmm.

If we're going down this road... For consistency's sake, I need ctrl+alt+9 to focus the last tab. That's how firefox, chromium, chrome, edge, legacy edge, and a bunch of other tabbed interfaces do it.

@DHowett
Copy link
Member

DHowett commented Jul 13, 2021

What's weird actually is how ^9 works in browsers. It is never tab 9. It is always "last tab"!

@DHowett
Copy link
Member

DHowett commented Jul 13, 2021

Well, it is tab 9 if the last tab is tab 9. I should not have said "never."

@zadjii-msft
Copy link
Member Author

If we're going down this road... For consistency's sake, I need ctrl+alt+9 to focus the last tab. That's how firefox, chromium, chrome, edge, legacy edge, and a bunch of other tabbed interfaces do it.

I mean, that's not too hard - setting it to

        { "command": { "action": "switchToTab", "index": 99999 }, "keys": "ctrl+alt+9" },

should work 😋

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jul 21, 2021
lhecker
lhecker approved these changes Jul 22, 2021
View reviewed changes
src/cascadia/TerminalApp/TabManagement.cpp
// GH#9369 - if the argument is out of range, then clamp to the number
// of available tabs. Previously, we'd just silently do nothing if the
// value was greater than the number of tabs.
tabIndex = std::clamp(tabIndex, 0u, _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.

tabIndex can't be smaller than 0, so you can also use std::min.

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

ghost commented Jul 22, 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 335f69e into main Jul 22, 2021
@ghost ghost deleted the dev/migrie/b/6776-focus-tab-clamping branch July 22, 2021 13:48
Rosefield pushed a commit to Rosefield/terminal that referenced this pull request Jul 22, 2021
@zadjii-msft @Rosefield
Clamp the focusTab action to the number of available tabs (microsof…
eb04dbb 
…t#10651)

## Summary of the Pull Request

When we perform a `focusTab` action, we currently do nothing if the parameter was greater than the number of tabs. This PR changes that behavior. Now, `focus-tab -t 999999` will always focus the last tab, instead of silently doing nothing. 

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

## Validation Steps Performed
* [x] ran tests
* [x] validated commandline manually
@ghost
Copy link

ghost commented Aug 31, 2021

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

Handy links:

@ghost ghost mentioned this pull request Aug 31, 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

@DHowett DHowett DHowett approved these changes

@lhecker lhecker lhecker 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-Commandline wt.exe's commandline arguments AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Focus the last tab when the param to focus-tab is greater than the number of tabs
3 participants
@zadjii-msft @DHowett @lhecker