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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setting for tab closing order #66635

Merged
merged 7 commits into from Jan 18, 2019

Conversation

Projects
None yet
4 participants
@SimonEggert
Copy link
Contributor

SimonEggert commented Jan 16, 2019

Closes #57554, closes #43459

This is my first contribution to the open source world! 馃帀

  • Write tests for setting
  • Make tests use different settings
@msftclas

This comment has been minimized.

Copy link

msftclas commented Jan 16, 2019

CLA assistant check
All CLA requirements met.

@sandy081 sandy081 requested a review from bpasero Jan 17, 2019

@bpasero
Copy link
Member

bpasero left a comment

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

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

This comment has been minimized.

@bpasero

bpasero Jan 17, 2019

Member

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

This comment has been minimized.

@SimonEggert

SimonEggert Jan 17, 2019

Author Contributor

You are right, my bad!

@@ -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');

This comment has been minimized.

@bpasero

bpasero Jan 17, 2019

Member

@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

This comment has been minimized.

@SimonEggert

SimonEggert Jan 17, 2019

Author Contributor

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

@@ -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') {

This comment has been minimized.

@bpasero

bpasero Jan 17, 2019

Member

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

This comment has been minimized.

@SimonEggert

SimonEggert Jan 17, 2019

Author Contributor

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鈥
鈥e method-calls, only calculate setting when config changes, adjust tests
@SimonEggert

This comment has been minimized.

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

2 checks passed

VS Code #20190117.56 succeeded
Details
license/cla All CLA requirements met.
Details
@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 18, 2019

Thanks 馃憤

@Somebi

This comment has been minimized.

Copy link

Somebi commented Feb 4, 2019

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

@Somebi

This comment has been minimized.

Copy link

Somebi commented Feb 9, 2019

thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment