Add option to move editors with `workbench.editor.revealIfOpen`

[jonathanjameswatson]

Adds the workbench.editor.revealIfOpenInActiveGroup configuration option, which simply changes workbench.editor.revealIfOpen to make any already-opened editor in a non-active editor group move to the active editor group on reveal. This makes opening an editor match the behavior of Emacs and Vim in a general and purely opt-in way.

Please let me know if there is anything I can do to help getting this PR merged.

Resolves #204942.

Demo:

https://github.com/microsoft/vscode/assets/23344719/0598e320-1e7d-43c4-a1cc-ab7b372c139d

[jonathanjameswatson]

@microsoft-github-policy-service agree

[jonathanjameswatson]

@aguynamedben

The only other thing I can think of is to update Working without Tabs in the docs to something like:

(I think this fits best under the "Working without Tabs" section.)

Simulating Buffers

If you're coming from buffer-centric editors such as Emacs or vi, you can emulate that workflow with:

"workbench.editor.showTabs": none, // or 'single' "workbench.editor.revealIfOpen": true, "workbench.editor.revealIfOpenInActiveGroup": true

VS Code has a model where Editors live hierarchically within Editor Groups, and to split your screen you must use Editor Groups. Using revealIfOpen with revealIfOpenInActiveGroup causes Quick Open to move already-open Editors into the active Editor Group. This allows you to emulate buffer-centric environments—no tabs, split your screen, and you can view any Editor within any Editor Group without opening a file multiple times.

I agree that we should add this in a PR in https://github.com/microsoft/vscode-docs if this PR gets accepted. I would also add that "workbench.editor.closeEmptyGroups": false could be good for an Emacs-like experience in the edge case of this feature moving an editor out of an editor group that only has one editor.

[aguynamedben]

@jonathanjameswatson Good call on closeEmptyGroups, I will prepare the docs PR so it's ready

[gjsjohnmurray]

I think moveToActiveGroupIfOpen would be a clearer name for the setting.

[jonathanjameswatson]

I think moveToActiveGroupIfOpen would be a clearer name for the setting.

OK, I will rename it quickly!

[jonathanjameswatson]

@gjsjohnmurray I have now renamed the configuration option and confirmed that it still works.

[bpasero]

I am not sure the editor group finder is the best place for this. It is not being used in all cases, only when the group is not specified explicitly.

We already have a location in our editor resolver where we look for an existing editor and move it conditionally:

https://github.com/microsoft/vscode/blob/9ccb0fa738a2f7a1c5328800c9fe18f4dd5b4f8a/src/vs/workbench/services/editor/browser/editorResolverService.ts#L505

This is for editors that can only be opened once for a resource, so I wonder if the option could apply here as well.

@lramos15 any objections?

[lramos15]

This is for editors that can only be opened once for a resource, so I wonder if the option could apply here as well.

This is sort of a special case that has to do with the inability to resolve the editor otherwise. I'm not sure the resolver should own user configurations about editor position since its main focus is just on editor types. Also then you wonder if other reveal related settings would move to the resolver? This is a small change so not a huge deal for now but I do worry if over time this will create code debt. Let me know your thoughts.

[bpasero]

I still think that this line is not 100% correct:

https://github.com/microsoft/vscode/pull/205442/files#diff-a9835b25a04cc268f3b704b319d5bfc15be4b0c85c2debe2708459b0726ea5feR161

We are in the process of trying to figure out which group to use and there we reference editorGroupService.activeGroup, but that is potentially not the group the user wants, for example when opening to the side. Thus I think this needs to be determined after the group has been figured out in the resolver or somewhere after the method was called.

[jonathanjameswatson]

It looks like you are right to say that this is not fully correct.

If the editor grid is:

Editor group 1: Editor A (active, visible), Editor B
Editor group 2: Editor C (visible)

Then opening editor B to the side will result in:

Editor group 1: Editor A (visible), Editor B
Editor group 2: Editor B (active, visible), Editor C

rather than:

Editor group 1: Editor A (visible)
Editor group 2: Editor B from editor group 1 (active, visible), Editor C

It definitely doesn't help that the line you mention only applies when preferredGroup is either undefined or ACTIVE_GROUP_TYPE, but I expect you are right that code outside of editorGroupFinder will still need to be changed even if this is fixed.

Something that I find interesting here is that if you have:

Editor group 1: Editor A (active, visible)
Editor group 2: Editor B, Editor C (visible)

and you open editor B to the side, you always seem to get:

Editor group 1: Editor A (active, visible)
Editor group 2: Editor B (visible), Editor C

even if workbench.editor.revealIfOpen is false. Is this the logic in the editor resolver that could be changed to work with moveToActiveGroupIfOpen?

[bethandtownes]

@jonathanjameswatson hi, thanks for this code change! I think this is going to make life a lot easier for vim/emacs users to onboard to vscode since will make vscode's editor concept to behave more like the buffer systems in those two editors.

@bpasero I saw this PR has been here for a while, wondering if there is anything we from open source community could do to help check this in?

[jonathanjameswatson]

@jonathanjameswatson hi, thanks for this code change! I think this is going to make life a lot easier for vim/emacs users to onboard to vscode since will make vscode's editor concept to behave more like the buffer systems in those two editors.

@bpasero I saw this PR has been here for a while, wondering if there is anything we from open source community could do to help check this in?

Hi, I do need to have another go at implementing this feature since the first attempt wasn't fully correct. If you would like to help, the first step would be to figure out a function that can be changed in a way that can handle all situations.

I think this could be done by putting some breakpoints in the editor resolver and experimenting with opening editors in different ways. For example, opening a definition to the side.

[jonathanjameswatson]

@bpasero I have now fixed the implementation of this feature by modifying the block of code that you linked to. I have checked that this feature works with opening to the side. It still works with editor group locking and having revealIfOpen enabled at the same time.

I made some changes to moveExistingEditorForResource which shouldn't affect its existing use case of handling singlePerResource editors. I have also documented that it should only take non-empty lists although perhaps we should also add an extra length check to the start. I also believe that the singlePerResource logic may currently ignore editor group locking. This behavior has been maintained in my PR but can be changed.

I check the configuration service in doResolveEditor now although it turns out there were already existing usages of the configuration service in editorResolverService. I could potentially change the function arguments here instead, although I think this would overall make the code style worse. Also, if you think it would be useful, I could add a change listener for moveToActiveGroupIfOpen so we don't call this.configurationService.getValue every time a file is opened?

[aguynamedben]

@bpasero Thank you for coaching through this change. Can you review? We need one more review for the check.