Add option to always reuse open editors #21815

Merged
merged 5 commits into from Mar 7, 2017

Conversation

Projects
None yet
4 participants
@ddotlic
Contributor

ddotlic commented Mar 2, 2017

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).

Add option to always reuse open editors
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 seems to be that we should apply the logic to all "open a document" actions).
@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Mar 2, 2017

@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

msftclas commented Mar 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 2, 2017

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

@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

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Mar 2, 2017

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

Thanks, Microsoft Pull Request Bot

msftclas commented Mar 2, 2017

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

Thanks, Microsoft Pull Request Bot

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

@bpasero bpasero self-assigned this Mar 3, 2017

@bpasero

Thanks, some feedback provided.

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

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

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 👍

@bpasero

bpasero Mar 3, 2017

Member

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 👍

This comment has been minimized.

@ddotlic

ddotlic Mar 3, 2017

Contributor

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.

@ddotlic

ddotlic Mar 3, 2017

Contributor

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.

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

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

@bpasero

bpasero Mar 3, 2017

Member

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

This comment has been minimized.

@ddotlic

ddotlic Mar 3, 2017

Contributor

@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

ddotlic Mar 3, 2017

Contributor

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

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

@ddotlic awesome

+ 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;

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

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

@bpasero

bpasero Mar 3, 2017

Member

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

This comment has been minimized.

@ddotlic

ddotlic Mar 3, 2017

Contributor

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).

@ddotlic

ddotlic Mar 3, 2017

Contributor

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).

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

Yes!

@@ -107,6 +107,11 @@ configurationRegistry.registerConfiguration({
'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': {

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

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.

@bpasero

bpasero Mar 3, 2017

Member

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.

This comment has been minimized.

@ddotlic

ddotlic Mar 3, 2017

Contributor

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 😳

@ddotlic

ddotlic Mar 3, 2017

Contributor

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 😳

This comment has been minimized.

@bpasero

bpasero Mar 3, 2017

Member

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

@bpasero

bpasero Mar 3, 2017

Member

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

Update based on the initial feedback.
Refactored according to @bpasero suggestions.
@ddotlic

This comment has been minimized.

Show comment
Hide comment
@ddotlic

ddotlic Mar 4, 2017

Contributor

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

Contributor

ddotlic commented Mar 4, 2017

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

@@ -944,6 +944,16 @@ export class EditorStacksModel implements IEditorStacksModel {
return this._groups.indexOf(group);
}
+ public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

Boolean => boolean

@bpasero

bpasero Mar 7, 2017

Member

Boolean => boolean

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

LOL, that was autocompleted by VS Code 😉

@ddotlic

ddotlic Mar 7, 2017

Contributor

LOL, that was autocompleted by VS Code 😉

@@ -944,6 +944,16 @@ export class EditorStacksModel implements IEditorStacksModel {
return this._groups.indexOf(group);
}
+ public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

activeOnly should probably be optional => activeOnly?: boolean

@bpasero

bpasero Mar 7, 2017

Member

activeOnly should probably be optional => activeOnly?: boolean

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

OK, no problem

@ddotlic

ddotlic Mar 7, 2017

Contributor

OK, no problem

@@ -944,6 +944,16 @@ export class EditorStacksModel implements IEditorStacksModel {
return this._groups.indexOf(group);
}
+ public findGroup(editorInput: EditorInput, activeOnly: Boolean): EditorGroup {
+ const found = this.groups.filter(group =>

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

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

bpasero Mar 7, 2017

Member

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)

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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.

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

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

@bpasero

bpasero Mar 7, 2017

Member

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

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

@ddotlic checking now...

@bpasero

bpasero Mar 7, 2017

Member

@ddotlic checking now...

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

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

@bpasero

bpasero Mar 7, 2017

Member

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

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

OK, understood.

@ddotlic

ddotlic Mar 7, 2017

Contributor

OK, understood.

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

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

@bpasero

bpasero Mar 7, 2017

Member

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

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

Fair enough.

@ddotlic

ddotlic Mar 7, 2017

Contributor

Fair enough.

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

=> if (group)

@bpasero

bpasero Mar 7, 2017

Member

=> if (group)

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

Thanks 👍

@bpasero

bpasero Mar 7, 2017

Member

Thanks 👍

This comment has been minimized.

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

@ddotlic

ddotlic Mar 7, 2017

Contributor

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

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

=> if (group)

@bpasero

bpasero Mar 7, 2017

Member

=> if (group)

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

This comment has been minimized.

@bpasero

bpasero Mar 7, 2017

Member

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.

@bpasero

bpasero Mar 7, 2017

Member

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.

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

ddotlic added some commits Mar 7, 2017

Incorporated feedback.
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

This comment has been minimized.

Show comment
Hide comment
@ddotlic

ddotlic Mar 7, 2017

Contributor

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

Contributor

ddotlic commented Mar 7, 2017

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

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

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

Member

bpasero commented Mar 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ddotlic

ddotlic Mar 7, 2017

Contributor

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

Contributor

ddotlic commented Mar 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

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

Member

bpasero commented Mar 7, 2017

@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 bpasero merged commit e415a78 into Microsoft:master Mar 7, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

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

Member

bpasero commented Mar 7, 2017

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

@ddotlic

This comment has been minimized.

Show comment
Hide comment
@ddotlic

ddotlic Mar 7, 2017

Contributor

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

Contributor

ddotlic commented Mar 7, 2017

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

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

Hehe ;)

Member

bpasero commented Mar 7, 2017

Hehe ;)

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