Skip to content

Add setting for tab closing order#66635

Merged
bpasero merged 7 commits intomicrosoft:masterfrom
SimonEggert:setting-for-closing-tabs
Jan 18, 2019
Merged

Add setting for tab closing order#66635
bpasero merged 7 commits intomicrosoft:masterfrom
SimonEggert:setting-for-closing-tabs

Conversation

@SimonEggert
Copy link
Contributor

@SimonEggert SimonEggert commented Jan 16, 2019 •
edited
Loading

Closes #57554, closes #43459

This is my first contribution to the open source world! 🎉

  • Write tests for setting
  • Make tests use different settings

@msftclas
Copy link

msftclas commented Jan 16, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@sandy081 sandy081 requested a review from bpasero January 17, 2019 14:52
bpasero
bpasero requested changes Jan 17, 2019
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@SimonEggert good version. I wonder though, do we need 2 different settings "Left to Right" and "Right to Left"? Wouldn't a boolean setting be easier to just disable the MRU behaviour?

src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated
oldOptions.labelFormat !== newOptions.labelFormat ||
oldOptions.tabCloseButton !== newOptions.tabCloseButton ||
oldOptions.tabSizing !== newOptions.tabSizing ||
oldOptions.tabClosingOrder !== newOptions.tabClosingOrder ||
Copy link
Member

@bpasero bpasero Jan 17, 2019

Choose a reason for hiding this comment

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

@SimonEggert all the changes in this file seem unneeded. We neither need to redraw the tab nor change classes when this setting changes, no?

Copy link
Contributor Author

@SimonEggert SimonEggert Jan 17, 2019

Choose a reason for hiding this comment

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

You are right, my bad!

src/vs/workbench/common/editor/editorGroup.ts Outdated
// More than one editor
if (this.mru.length > 1) {
this.setActive(this.mru[1]); // active editor is always first in MRU, so pick second editor after as new active
const tabClosingOrder = this.configurationService.getValue('workbench.editor.tabClosingOrder');
Copy link
Member

@bpasero bpasero Jan 17, 2019

Choose a reason for hiding this comment

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

@SimonEggert I would prefer if this setting was put as variable similar to private editorOpenPositioning so that we do not have to compute it each time

Copy link
Contributor Author

@SimonEggert SimonEggert Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the hint, I already thought there was a better way to do this.

src/vs/workbench/common/editor/editorGroup.ts Outdated
if (this.mru.length > 1) {
this.setActive(this.mru[1]); // active editor is always first in MRU, so pick second editor after as new active
const tabClosingOrder = this.configurationService.getValue('workbench.editor.tabClosingOrder');
if (tabClosingOrder === 'rtl') {
Copy link
Member

@bpasero bpasero Jan 17, 2019

Choose a reason for hiding this comment

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

@SimonEggert instead of repeating this.setActive... so many times, can we just store the editor as variable and then call this method last with the right one?

Copy link
Contributor Author

@SimonEggert SimonEggert Jan 17, 2019

Choose a reason for hiding this comment

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

I adjusted the code to avoid multiple this.setActive-calls.

@bpasero bpasero added this to the On Deck milestone Jan 17, 2019
Change setting to boolean, remove unneeded code changes, avoid multip…
030e4f9 
…le method-calls, only calculate setting when config changes, adjust tests
@SimonEggert
Copy link
Contributor Author

SimonEggert commented Jan 17, 2019

@bpasero Thank you for your feedback and tips. I tried my best to implement the changes you requested, so feel free to take a look and let me know what you think.

@bpasero bpasero merged commit f85db57 into microsoft:master Jan 18, 2019
@bpasero
Copy link
Member

bpasero commented Jan 18, 2019

Thanks 👍

This was referenced Jan 18, 2019
Closed
Closed
@Somebi
Copy link

Somebi commented Feb 4, 2019

So is this feature implemented? How it's called?

@bpasero
Copy link
Member

bpasero commented Feb 4, 2019

https://github.com/Microsoft/vscode-docs/blob/vnext/release-notes/v1_31.md#closing-order-of-editor-tabs

@Somebi
Copy link

Somebi commented Feb 9, 2019

thanks ;)

@myfonj
Copy link

myfonj commented Nov 21, 2019

@bpasero

[..] do we need 2 different settings "Left to Right" and "Right to Left"? Wouldn't a boolean setting be easier to just disable the MRU behaviour?

This remark apparently made OP completely remove much desired right to left closing order option (?).

Logically, this should have been three state option controlling what happens after tab is closed.
After closing editor, focus moves to:

  • Most recently used editor a.k.a. "MRU order", current default;
  • Right (next) adjacent editr, a.k.a. "LTR order", current "workbench.editor.focusRecentEditorAfterClose"=false;
  • Left (previous) adjacent editor, a.k.a. "RTL order", currently not possible.

Issue #67730 (Close with ctrl+w from right to left and not vice-versa) called for last mentioned option, but was mistakenly marked as duplicate of #57554 (Allow to close tabs from left to right [rather than MRU]) what was resolved by this PR.

So option for right to left closing order is still missing but would be really nice.

@bpasero
Copy link
Member

bpasero commented Dec 5, 2019

Feel free to open a new issue for it.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bpasero bpasero bpasero requested changes

Assignees

@bpasero bpasero

Labels

None yet

Projects

None yet

Milestone

December/January 2019

Development

Successfully merging this pull request may close these issues.

Allow to close tabs from left to right rather than in order of most recently focused Make closing tab to open next/previous tab instead of MRU

5 participants

@SimonEggert @msftclas @bpasero @Somebi @myfonj