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

Add setting for tab closing order #66635

Merged
merged 7 commits into from
Jan 18, 2019
Merged

Add setting for tab closing order #66635

merged 7 commits into from
Jan 18, 2019

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
@@ -385,6 +385,7 @@ export class TabsTitleControl extends TitleControl {
oldOptions.labelFormat !== newOptions.labelFormat ||
oldOptions.tabCloseButton !== newOptions.tabCloseButton ||
oldOptions.tabSizing !== newOptions.tabSizing ||
oldOptions.tabClosingOrder !== newOptions.tabClosingOrder ||
Copy link
Member

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

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
@@ -332,7 +332,26 @@ export class EditorGroup extends Disposable {

// 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

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
@@ -332,7 +332,26 @@ export class EditorGroup extends Disposable {

// 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');
if (tabClosingOrder === 'rtl') {
Copy link
Member

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

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
@SimonEggert
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

@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
5 participants
@SimonEggert @msftclas @bpasero @Somebi @myfonj