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

fix: make on first tab cycle back to last tab #11794

Conversation

Pranav2612000
Copy link
Contributor

@Pranav2612000 Pranav2612000 commented Oct 22, 2022

What it does

Fixes #11746

Before this commit, trying to cycle back to the previous tab, when you were on the first tab had no action. The expected behaviour was to have the last tab in focus. This was because trying to move back from the first tab was thought to be an action of moving to the previous tab group. This commit fixes that issue by treating it as a same tab group action, and redirecting the user to the last tab in the same group.

How to test

Review checklist

Reminder for reviewers

@Pranav2612000
Copy link
Contributor Author

@colin-grant-work Can you take a look at this PR when you have time

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

@Pranav2612000 Pranav2612000 force-pushed the fix/11746-cycle-back-to-last-tab-on-moving-back branch from 19a91d2 to 32af705 Compare October 24, 2022 14:02
Before this commit, trying to cycle back to the previous tab, when you
were on the first tab had no action. The expected behaviour was to have
the last tab in focus. This was because trying to move back from the first
tab was thought to be an action of moving to the previous tab group. This
commit fixes that issue by treating it as a same tab group action, and
redirecting the user to the last tab in the same group.

Fixes eclipse-theia#11746

Signed-off-by: Pranav Joglekar <[email protected]>
@Pranav2612000 Pranav2612000 force-pushed the fix/11746-cycle-back-to-last-tab-on-moving-back branch from 32af705 to 430437e Compare October 24, 2022 14:05
@Pranav2612000
Copy link
Contributor Author

I've signed the agreement. Thanks for also updating the issue description. I'll keep it in mind in the future.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 👍 I confirmed that:

  • cycling available tabs in a group using view: show previous tab works as expected (#11746)
  • closing a tab in a different group will preserve the currently opened tab (#10865)

I'd like to get the feedback of others before merging to see if any regressions are present, but it may take a bit of time since they are at the EclipseCon 2022 conference this week.

@vince-fugnitto vince-fugnitto added the shell issues related to the core shell label Nov 2, 2022
@perrinjerome
Copy link
Contributor

thank you @Pranav2612000 I hope this can make it for the next release

@vince-fugnitto
Copy link
Member

I believe that @colin-grant-work reviewed the pull-request and noticed issues with the approach, the previous tab command would always switch to an adjacent tab, and not respect the precedence of which tab was last focused.

@Pranav2612000
Copy link
Contributor Author

I thought this was the expected approach. Can you help me with the exact reproduction so I can fix it.

@vince-fugnitto
Copy link
Member

@Pranav2612000 I believe that @colin-grant-work expressed an oddity with this approach but I'll let him comment further when he's back. As for now I think it does improve the situation and has value.

@vince-fugnitto vince-fugnitto merged commit 5e51cbd into eclipse-theia:master Nov 24, 2022
@colin-grant-work
Copy link
Contributor

The oddity relates not so much to this case as what happens when we close views and invoke activatePreviousTabBar. In that case, the tabbar that is activated is always adjacent to the current tabbar, whereas in VSCode the tabbars are prioritized by recency. I will make a separate issue for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell issues related to the core shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View: Show Previous Tab on first tab no longer cycle to last tab
4 participants