Open Editor refactors

[lramos15]

This PR is part of the work for #100281

The main work of this PR includes:

1. Renaming IEditorInputFactory -> IEditorInputSerializer as it doesn't actually create an editor input at all
2. Move the override logic into the editor service and away from the editorGroupView. This involved removing the onEditorWillOpen event as it wasn't really necessary. An improvement here might be to have the overrides return editor inputs to prevent circular openEditor calling
3. Create an onWillMoveEditor event so things like notebooks can properly handle moving between groups

[bpasero]

I really like these changes, good job. I focused mainly on the editor related changes and for notebook + custom defer to your expertise because I am not comfortable reviewing changes in those areas.

Here is some general feedback:

• there is a ton of places now where we have the IEditorInputSerializer but the surrounding context often still refers to it as a "factory". I think for consistency and avoiding confusion in the future I would also rename everything around it to refer to "serializers" and not "factories" anymore. does that sounds reasonable? this probably means to go over the change again step by step and doing a bit more renames but I think that would be worth it especially since we may want to introduce editor input factory in the future which then collides with the serializer concept
• I see some commented out tests in editorGroupsService.test.ts and think that they need to be converted in a way that we can test the new concepts you introduced (e.g. onWillMoveEditor)

[lramos15]

@bpasero This is ready for review again. The one issue @rebornix and I ran into is that on editor startup / restore there is no way for something to know about all the groups that exist on startup. I changed the editor part restore to fire the onAddGroup event for all the restored groups so any subscribers would be aware of them but I want to get your input as this causes active editor tests to fail so I'm not sure if they should be updated or what the best solution is.

[bpasero]

@lramos15 I did a review and pushed some 💄 changes on top that I had in mind from my first review, I hope that is fine.

There is slight inconsistency with this change that we may want to tackle later:

• override is still a member of EditorOptions but is actually ignored unless you go through the IEditorService, so maybe we should actually remove it from the options and explicitly add it as parameter to the IEditorOpenService#openEditor method alone?
• the moveEditor method does not really support all the editor options passed unless the editor moves to another group but I think that is fine, I removed the move/copy options bag and replaced it with the editor options interface we already have

there is no way for something to know about all the groups that exist on startup

I signed off on all changes, except for the one that is causing test failures and maybe can we push a more isolated fix only for notebooks? It is funny that you ran into this issue because I have #119059 (comment) where I face a similar problem where my editor observer is only starting to track editor groups after editors have restored but this causes all kinds of issues. In April I want to push a change to have a way to track editor groups even before editors restore. In other words: the editor part should signal editors have been restored before the actual editors have been loaded. This should actually enable us to track groups and editor s right on startup. But I am a bit afraid to push this right before endgame.

Where do notebooks need the groups right on startup?

[lramos15]

Thanks for the review. The 💄 looks good too, sorry about forgetting to rename the areas around IEditorInputSerializer. I talked to Kai a bit and he said we should probably hold off on merging this until next iteration just due to how little testing time we would have with these changes. This should give us time to come up with a good solution for your issue that also unblocks notebooks. I don't want to merge this yet as notebooks not knowing about all groups causes the re-rendering issue we were trying to avoid with onWillMoveEditor.

Here you can see that notebooks binds event listeners to all groups, but due to not knowing about groups on startup / restore reloading the window is enough to cause all existing listeners to be lost and not rebound.

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

Lines 40 to 64 in a3d388e

	const onNewGroup = (group: IEditorGroup) => {
	const { id } = group;
	const listeners: IDisposable[] = [];
	listeners.push(group.onDidGroupChange(e => {
	const widgets = this._borrowableEditors.get(group.id);
	if (!widgets || e.kind !== GroupChangeKind.EDITOR_CLOSE || !(e.editor instanceof NotebookEditorInput)) {
	return;
	}
	const value = widgets.get(e.editor.resource);
	if (!value) {
	return;
	}
	value.token = undefined;
	this._disposeWidget(value.widget);
	widgets.delete(e.editor.resource);
	}));
	listeners.push(group.onWillMoveEditor(e => {
	if (e.editor instanceof NotebookEditorInput) {
	this._freeWidget(e.editor, e.source, e.target);
	}
	}));
	groupListener.set(id, listeners);
	};
	this._disposables.add(editorGroupService.onDidAddGroup(onNewGroup));
	editorGroupService.groups.forEach(onNewGroup);

[bpasero]

@lramos15 I don't really understand why notebooks cannot do this:

editorGroupService.whenRestored.then(() => editorGroupService.groups.forEach(onNewGroup)); 

There should always be a way to track editor groups, in the end that is what EditorsObserver does, which is a core thing crucial for many features we built on top:

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

Lines 67 to 73 in 763e089

	private registerListeners(): void {
	this._register(this.storageService.onWillSaveState(() => this.saveState()));
	this._register(this.editorGroupsService.onDidAddGroup(group => this.onGroupAdded(group)));
	this._register(this.editorGroupsService.onDidChangeEditorPartOptions(e => this.onDidChangeEditorPartOptions(e)));
	this.editorGroupsService.whenRestored.then(() => this.loadState());
	}

[lramos15]

@bpasero I have fixed the tests and adjusted the notebook's way of listening for groups to the way you suggested and that seems to work.

I noticed you renamed the notebook and custom editor input factory files to input serializer files. I'm not sure what we could call those as they have both an IEditorInputSerializer and a ICustomEditorInputFactory

[bpasero]

@lramos15 very cool! yeah feel free to revert my rename, I didn't see they include the actual custom factories too 👍

[bpasero]

Ok for me to merge in April 👍