Revisit lifecycle of editor inputs in VSCode

[bpasero]

Stumbled upon this code (after chat with @jrieken):

vscode/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts

Lines 216 to 230 in 19dcc79

	this._register(this.editorService.onDidCloseEditor(({ editor }) => {
	if (!(editor instanceof NotebookEditorInput)) {
	return;
	}
	if (!this.editorService.editors.some(other => (
	other.resource === editor.resource
	&& other instanceof NotebookEditorInput
	&& other.viewType === editor.viewType
	))) {
	editor.clearTextModel();
	}
	editor.dispose();
	}));

The editor part should already take care of disposal. You can see it here:

vscode/src/vs/workbench/browser/parts/editor/editorGroupView.ts

Lines 535 to 537 in 19dcc79

	if (!this.accessor.groups.some(groupView => groupView.group.contains(editorToClose, true /* include side by side editor master & details */))) {
	editorToClose.dispose();
	}

We call EditorInput.dispose() when the last kind of that editor is closed across all groups. This is in the end determined by the EditorInput.matches() method and goes like this:

• for all groups
• for all editors in the group
• check if the editor.matches(editorThatClosed)

I see that this may result in editor inputs not getting disposed in the following case:

• user opens notebook foo
• user splits the editor and a second notebook foo is opened
• at this time we have 2 notebook inputs
• user closes one notebook
• we do not dispose the first notebook input because we find that it is still opened (via EditorInput.matches())

This is unfortunate and should be fixed by always ensuring the same editor input is used for the same underlying resource (something we do for files always). The editor part cannot really enforce this, so I wonder if a better idea would be to introduce a method such as EditorInput#close() that we always call when an editor is being closed and then each input has to decide how to deal with it.

[bpasero]

@mjbvz curious to hear your thoughts on this since you are a big customer of editor inputs as well. Do you share inputs of same resources (for webviews, custom editors?). How about the lifecycle there?

[mjbvz]

@bpasero I believe we avoid this for webviews and custom editors because we create a unique editor input for each copy of the editor that is open (even if they are for the same resource):

vscode/src/vs/workbench/contrib/customEditor/browser/customEditors.ts

Line 487 in e015178

// Unlike normal editor inputs, we do not want to share custom editor inputs

We do this because WebviewEditorInput stores the <webview> element, which cannot be shared between multiple editors.

[bpasero]

@mjbvz so are you implementing the matches method in a way that accounts for this? Otherwise your inputs are not getting disposed?

I am really thinking we should have an explicit close method on the editor inputs and let inputs deal with the dispose as it best fits. This method could carry an indicator if the input is still opened in any other group to prevent duplicate work.

[bpasero]

@rebornix @mjbvz via a18110a there is now a new method on editor inputs:

close(group: GroupIdentifier, openedInOtherGroups: boolean): void {
	if (!openedInOtherGroups) {
		this.dispose();
	}
}

For now it preserves exactly the behaviour we had.

In the future this should possibly be an abstract method as I think only an implementor of an editor input knows if the input can be disposed or not.

@rebornix does that help in removing the editor close listener you have to setup?

[bpasero]

This is a list of all our editor inputs in VSCode where we should check for adoption:

Maybe @mjbvz you could make a statement about custom editors and webview related ones.

[jrieken]

re #99770 (comment)

So, EditorInput#close is called whenever a tab goes away? Is that also true for moving tabs?

[rebornix]

@bpasero Speaking of EditorInput, we create editor input for every editor tab even if they might be the same resources, which is the same as the CustomEditor/Webview. The major reason for this design is we want to maintain a 1 to 1 mapping between Editor (Tab) to NotebookEditorWidget. If there are two editor tabs for the same resource, there will be two NotebookEditorWidget. If users move one editor to another group, we will reuse the NotebookEditorWidget from previous editor input. That's also why we implement our own matches API on editor input.

If we are going to share EditorInput between the same resource in the multiple groups, then we need to maintain the 1:1 mapping between group and NotebookEditorWidget. However, we can't store it in EditorInput as it's group agnostic, and also we don't know if an editor is moving to another group (meaning we don't know which widget to reuse when opening an EditorInput in a new group).

update

I added proposals and deleted it three times after playing around editorGroupView and EditorInput. I'll suggest we have a sync online for more detailed discussions.

[jrieken]

I'll suggest we have a sync online for more detailed discussions.

Yeah - I have ref counted notebook models follow the re-use editor input approach but it's impossible for to get the "share widget" logic to work. The challange is that is all cases Editor#setInput is called first, e.g before group#onWillCloseEditor or clearInput etc. Only the open-editor-overwrite can do that but it has no other way of identifying editors than the input. Which is sometimes shared and sometimes not shared.

[bpasero]

@jrieken

So, EditorInput#close is called whenever a tab goes away? Is that also true for moving tabs?

Yes, but the logic for moving a tab will always first open it in the second group and then close it in the first group. So you will end up getting a close call but openedInOtherGroups will be true because the tab is already opened there:

It is fine if you decide to NOT share editor inputs, this is not a requirement of the editor model at all. Just files decided to do this.

I hope the new close method makes the lifecycle easier to adopt. The editor part will no longer just dispose any inputs or make assumptions.

[jrieken]

It is fine if you decide to NOT share editor inputs, this is not a requirement of the editor model at all.

Yeah, I came to the same conclusion and I'll drop my current (branched) changes. One challenge is that the workbench defaults to reuse and that notebooks and custom editors fight against that. So, could we have workbench support here, e.g when splitting an editor check for EditorInput#clone and iff that exists call that?

[bpasero]

@jrieken

One challenge is that the workbench defaults to reuse

Is that only for splitting or are there more places where you see that?

I think it is fine to add more methods to EditorInput to solve this, e.g. we already have that method supportsSplitEditor and I think we could change it to return the editor input to use when splitting or undefined if unsupported 👍

[jrieken]

we already have that method supportsSplitEditor and I think we could change it to return the editor input to use when splitting or undefined if unsupported 👍

Oh, yeah I like that

[bpasero]

So after looking into this again going through all editors, I see that this is not so trivial. I still think a close method is good and that the editor part should not simply dispose editor inputs. And I think it is good to put the dispose control into the hands of the editor input implementation.

However, the logic that we have today that we only dispose an editor input if it is not opened in other groups is problematic. Specifically this code:

close(group: GroupIdentifier, openedInOtherGroups: boolean): void {
	if (!openedInOtherGroups) {
		this.dispose();
	}
}

Here is why:

• today some editors decide to share the same instance across all groups or not, for example files do but notebooks or custom editors don't
• however this is NOT enforced end to end, you can actually open the same editor in another editor group, following these steps [1]
• if an editor decides to NOT share editor inputs across groups, your input will not be disposed when you close it if it is opened in another group (this is essentially Editor of first group not disposed #99887). you can now in theory fix this by overriding the close method, however if you follow steps from [1] you might end up disposing the same editor that is still opened in another group

[1] Open same editor across groups

• open an editor such as notebook or custom editor
• open an empty editor group to the side
• focus that empty editor group
• bring up editor history (Cmd+P) and select the notebook or custom editor
  => you have now opened the same editor input in another group, even if that editor configured that it does not support splitting

Bottom line: a call between custom editor and notebook owners would be good to collect requirements and then go from there. I will hold to push anymore changes on this matter until we decided what is needed.

[bpasero]

Actually how would we enforce this, e.g. someone could easily write the following code:

const input = new MyInput();
editorService.openEditor(input, groupA);
editorService.openEditor(input, groupB);

I feel that we need to make sure that editor inputs really remain lightweight and do not carry anything heavy around. They should only be used to instruct the editor part to open something imho.

[mjbvz]

Some quick high level note on expected behavior for webviews and custom editors.

Webviews

• Every webview is unique.
• There should ever be one instance of a given webview across all editors.
• Splitting is not allowed.
• Once closed, a webview should not be able to be reopened. (This is broken today)

Custom Editors that set supportsMultipleEditorsPerDocument:false

• There should only ever be one custom editor for this resource across all editors.
• Custom Editors can be reopened after they are closed.

Normal Custom editors (that set supportsMultipleEditorsPerDocument: true)

• There can only be one custom editor for a resource open in a given editor group
• However there can be multiple editors for this resource across all editor groups
• But all of these editors have their own unique state (they share a document model). This is because each editor instance has its own unique <webview> element. We try to preserve this webview element as much as possible when the user moves around editors

[bpasero]

The editor part changed its strategy for calling dispose in a way that should make more sense now:

• previously: IEditorInput.dispose() was only called if the input was not opened in another group by calling IEditorInput.matches()
• now: IEditorInput.dispose() is called if the input is not opened in another group using === checks

As such, inputs should now be more aggressively disposed if they decide to not share the same instance across groups.

As such, this special code for custom editors was removed: 063ea11