Add option to always reuse open editors by ddotlic · Pull Request #21815 · microsoft/vscode

ddotlic · 2017-03-02T18:40:26Z

The idea being that we never* open the same file in more than one editor at a time, regardless of the action which opens the editor.

*never meaning whenever possible, for example splitting with a single file open or explicitly opening the same file "side by side" cannot work if we literally take the newly introduced option.
Implements #7049 (note that the issue's title talks only about Ctrl/Cmd+P action but the consensus - at least with @bpasero and the issue creator - seems to be that we should apply the logic to all "open a document" actions).

The idea being that we never* open the same file in more than one editor at a time, regardless of the action which opens the editor. *never meaning whenever possible, for example splitting with a single file open or explicitly opening the same file "side by side" cannot work if we literally take the newly introduced option. Implements microsoft#7049 (note that the issue's title talks only about Ctrl/Cmd+P action but the consensus seems to be that we should apply the logic to all "open a document" actions).

msftclas · 2017-03-02T18:40:30Z

@ddotlic,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

mention-bot · 2017-03-02T18:40:36Z

@ddotlic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @egamma to be potential reviewers.

msftclas · 2017-03-02T18:44:40Z

@ddotlic, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

bpasero

Thanks, some feedback provided.

bpasero · 2017-03-03T10:44:49Z

src/vs/workbench/browser/parts/editor/editorPart.ts

+		// Respect option to reveal an editor if it is open (not necessarily already visible)
+		const skipReuse = (options && options.index) || arg1;
+		if(!skipReuse && reuseIfOpen) {
+			const groups = this.stacks.groups;


I suggest to extract this into a method findGroup(editor) in the EditorStacksModel and add tests for it. I would add an option to this method to only respect the active editor and then also use it for the revealIfVisible option further below 👍

I initially went down this road, but the code became less readable. It's gonna be easier to show you by implementing it than to discuss, let's revisit.

I would be surprised and the added value of being able to write tests is a big plus.

@bpasero Sorry, my comment was related only to the value of extracting into a findGroup method, not adding tests for it. I did not mean to imply there's no value in adding tests.
Let's do it like this: I'll extract into a method and if you think it's readable, I'll proceed with the tests.

@ddotlic awesome

bpasero · 2017-03-03T10:47:00Z

src/vs/workbench/browser/parts/editor/editorPart.ts

+		const config = this.configurationService.getConfiguration<IWorkbenchEditorConfiguration>();
+		const reuseIfOpen = config.workbench.editor.reuseIfOpen;
+		// Respect option to reveal an editor if it is open (not necessarily already visible)
+		const skipReuse = (options && options.index) || arg1;


There are imho more cases where you need to skipReuse: when revealIfVisible is true and there is a visible editor matching the input you should prefer it

If so, wouldn't this be handled simply by moving the revealIfVisible code block before the newly introduced one? It exits early so we don't need to check for it later on (assuming I move the new code block below).

bpasero · 2017-03-03T10:52:26Z

src/vs/workbench/electron-browser/main.contribution.ts

 			'default': 'right',
 			'description': nls.localize({ comment: ['This is the description for a setting. Values surrounded by single quotes are not to be translated.'], key: 'editorOpenPositioning' }, "Controls where editors open. Select 'left' or 'right' to open editors to the left or right of the current active one. Select 'first' or 'last' to open editors independently from the currently active one.")
 		},
+		'workbench.editor.reuseIfOpen': {


The name and description reads a bit weird, maybe this should be called revealIfOpen? Because the editor is not reused if opened, it is being revealed. That also aligns with the revealIfVisible option.

Description idea:

Controls if an editor is revealed in any of the visible groups if opened. If disabled, an editor will prefer to open in the currently active editor group. If enabled, an already opened editor will be revealed instead of opened again in the currently active editor group. Note that there are some cases where this setting is ignored, e.g. when forcing an editor to open in a specific group or to the side of the currently active group.

I am in no way pretending I got the wording right 😄 I'm perfectly fine putting in whatever you feel is suitable (the above IMHO is good enough, if a bit long).
I struggled quite a bit with the option name and couldn't think of anything obviousy good, so thanks for the suggestion 👍
This being my first PR in a while, I'll need to first figure out how to update this PR instead of sending a new one 😳

No worries. To update this PR you just push to your branch ddotlic:master. Thats it.

@bpasero

Refactored according to @bpasero suggestions.

ddotlic · 2017-03-04T19:07:23Z

@bpasero Updated the PR with the changes you've requested. It's all there, including unit test for the code we've extracted into a common method.

Let me know if I can do something more to make this acceptable. Thanks!

bpasero

Thanks, first round of feedback provided.

bpasero · 2017-03-07T06:15:27Z

src/vs/workbench/common/editor/editorStacksModel.ts

 		return this._groups.indexOf(group);
 	}

+	public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {


Boolean => boolean

LOL, that was autocompleted by VS Code 😉

bpasero · 2017-03-07T06:16:01Z

src/vs/workbench/common/editor/editorStacksModel.ts

 		return this._groups.indexOf(group);
 	}

+	public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {


activeOnly should probably be optional => activeOnly?: boolean

OK, no problem

bpasero · 2017-03-07T06:18:39Z

src/vs/workbench/common/editor/editorStacksModel.ts

 	}

+	public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {
+		const found = this.groups.filter(group =>


I suggest to change this logic a) for perf reasons and b) for logical reasons

a) Perf: I suggest to simply use 2 for loops here to avoid iterating over all groups and all editors even if a match has found. With the current logic, if the editor is found in group 1 you will still iterate over all other groups and editors even though we only return one result.

b) Logic: there is always an active group and that group might not be group 1. Wouldn't it make more sense to start with the currently active group to look for the editor and then check the others? This means we prefer to reveal an editor in the currently active group if we find it. Another solution would be to prefer an editor if it is already visible in one of the groups. You have to decide after all how this option should behave:

does it prefer active editors over inactive

does it prefer active groups over inactive

does it prefer strict spatial order of the groups (left to right)

a) I initially wrote it as such, precisely for the reasons you listed, but it somehow looked ugly and the intent was obscured by lots of lines of "useless" code; I decided to err on the side of readability seeing how we're talking about 3 groups at most and what, couple dozen editors open? But if you prefer perf, that's what we'll do
b) Not really, it all depends on the activeOnly parameter; if I start catering for that (as it was previously), we'll end up with a mess - code which tries to do two things at the same time, which is how I originally ended up leaving it as-is and just adding a secondary 2 for loops (exactly what you wanted for this method btw)

If I understand what you're saying, the best approach would be to simply take my previous code and paste it into the method. It's gonna be much longer, less readable but faster in the most common case.

@bpasero The code before assumed that we prefer active editor and groups over inactive as it made sure to first check the active ones.

I suggest you come up with something that you think makes most sense and I review it again 👍

@bpasero It's all done since an hour ago - don't you see my changes?

@ddotlic checking now...

bpasero · 2017-03-07T06:19:31Z

src/vs/workbench/common/editor/editorStacksModel.ts

+				return editor.matches(editorInput)
+					&& (!activeOnly || isActive);
+			}));
+		return found[0] || null;


It is fine to return undefined instead of null, you do not need to do an explicit || null.

OK, understood.

bpasero · 2017-03-07T06:21:05Z

src/vs/workbench/common/editor/editorStacksModel.ts

+		const found = this.groups.filter(group =>
+			group.getEditors().some(editor => {
+				const isActive = group.isActive(editor);
+				return editor.matches(editorInput)


You should first check for the isActive condition and then do the editor.matches call to avoid it unless needed.

Fair enough.

bpasero · 2017-03-07T06:21:45Z

src/vs/workbench/browser/parts/editor/editorPart.ts

+		const skipReuse = (options && options.index) || arg1;
+		if (!skipReuse && revealIfOpen) {
+			const group = this.stacks.findGroup(input, false);
+			if (null !== group) {


=> if (group)

OK, let me do the changes quickly as today I have a lot more bandwidth for this kind of work.

Thanks 👍

@bpasero Just to make sure it's understood, the changes have been done and the code pushed an hour ago, feel free to proceed with the review at your convenience.

bpasero · 2017-03-07T06:22:13Z

src/vs/workbench/browser/parts/editor/editorPart.ts

-					return editorToCheck.position;
-				}
+			const group = this.stacks.findGroup(input, true);
+			if (null !== group) {


=> if (group)

bpasero · 2017-03-07T06:23:52Z

src/vs/workbench/browser/parts/editor/editorPart.ts

+			}
+		}
+
+		const config = this.configurationService.getConfiguration<IWorkbenchEditorConfiguration>();


Instead of getting the config each time we open an editor I suggest to store the value in a field of EditorPart. You can see in the constructor that we resolve configuration and we also react to updates.

Note that at the expense of memory consumed, the portions of the code where we "order" groups and editors such that active one comes first could have been much more readable.

ddotlic · 2017-03-07T07:48:39Z

@bpasero Next time I'll make a branch for my changes, merging with master as we develop this PR is annoying at best 😳

bpasero · 2017-03-07T09:06:58Z

@ddotlic I merged with your master to be able to apply the diff in my VS Code instance (using git apply) since I cannot use GitHubs diff to review changes, it's just not possible for me. When I diff from within VS Code I can actually understand what is going on.

ddotlic · 2017-03-07T09:08:51Z

@bpasero I'm just saying that it's my fault for making your life difficult, sorry for that, having a separate branch in the future should help you see the diff much better.

bpasero · 2017-03-07T09:32:09Z

@ddotlic no problem at all, for me it was not harder compared to other PRs, I have my fixed list of steps to do to bring a PR up to speed with master to be able to create a patch I can apply :)

bpasero · 2017-03-07T09:47:55Z

@ddotlic thanks, merged. I will push some 💄 on top of it, nothing big.

ddotlic · 2017-03-07T09:49:14Z

@bpasero Let's hope it's not like in the saying regarding 💄 on a 🐷 🤣

bpasero · 2017-03-07T14:26:19Z

Hehe ;)

msftclas added the cla-required label Mar 2, 2017

msftclas added cla-signed and removed cla-required labels Mar 2, 2017

bpasero self-assigned this Mar 3, 2017

bpasero requested changes Mar 3, 2017

View reviewed changes

Update based on the initial feedback.

cc16487

Refactored according to @bpasero suggestions.

Merge remote-tracking branch 'upstream/master'

a7d1190

bpasero requested changes Mar 7, 2017

View reviewed changes

bpasero added this to the March 2017 milestone Mar 7, 2017

ddotlic added 2 commits March 7, 2017 08:44

Incorporated feedback.

69965b8

Note that at the expense of memory consumed, the portions of the code where we "order" groups and editors such that active one comes first could have been much more readable.

Merged branch master into master

7374986

bpasero merged commit e415a78 into microsoft:master Mar 7, 2017

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

ddotlic commented Mar 2, 2017

Uh oh!

msftclas commented Mar 2, 2017

Uh oh!

mention-bot commented Mar 2, 2017

Uh oh!

msftclas commented Mar 2, 2017

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddotlic commented Mar 4, 2017

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bpasero Mar 7, 2017 •

edited

Loading

bpasero commented Mar 7, 2017 •

edited

Loading