IOpenEditorOverrideHandler is very weird

[bpasero]

I feel IOpenEditorOverrideHandler is weird.

Why does open return a IOpenEditorOverride | undefined where IOpenEditorOverride#override is optional? Why can we not directly return Promise<IEditorPane | undefined> from open.

And what is the purpose of getEditorOverrides. It seems weird to me that a contributor can return IOpenEditorOverrideEntry[] but in the open call there are no traces anymore of what override entry was used.

[bpasero]

My hope is that maybe with #100281 all of this weird indirections could go away.

[jrieken]

Just for reference, the implementation of notebook's open. Maybe we aren't doing this right but I have the feeling this shouldn't be so much code

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

Lines 322 to 476 in 65e2832

	private onEditorOpening(originalInput: IEditorInput, options: IEditorOptions | ITextEditorOptions | undefined, group: IEditorGroup): IOpenEditorOverride | undefined {
	let id = typeof options?.override === 'string' ? options.override : undefined;
	if (id === undefined && originalInput.resource?.scheme === Schemas.untitled) {
	return undefined;
	}
	if (originalInput instanceof DiffEditorInput && this.configurationService.getValue(NotebookTextDiffEditorPreview) && !this._accessibilityService.isScreenReaderOptimized()) {
	return this._handleDiffEditorInput(originalInput, options, group);
	}
	if (!originalInput.resource) {
	return undefined;
	}
	// Run reopen with ...
	if (id) {
	// from the editor tab context menu
	if (originalInput instanceof NotebookEditorInput) {
	if (originalInput.viewType === id) {
	// reopen with the same type
	return undefined;
	} else {
	return {
	override: (async () => {
	const notebookInput = NotebookEditorInput.create(this.instantiationService, originalInput.resource, id);
	const originalEditorIndex = group.getIndexOfEditor(originalInput);
	await group.closeEditor(originalInput);
	originalInput.dispose();
	const newEditor = await group.openEditor(notebookInput, { ...options, index: originalEditorIndex, override: EditorOverride.DISABLED });
	if (newEditor) {
	return newEditor;
	} else {
	return undefined;
	}
	})()
	};
	}
	} else {
	// from the file explorer
	const existingEditors = this.editorService.findEditors(originalInput.resource, group).filter(editor => editor instanceof NotebookEditorInput) as NotebookEditorInput[];
	if (existingEditors.length) {
	// there are notebook editors with the same resource
	if (existingEditors.find(editor => editor.viewType === id)) {
	return { override: this.editorService.openEditor(existingEditors.find(editor => editor.viewType === id)!, { ...options, override: EditorOverride.DISABLED }, group) };
	} else {
	return {
	override: (async () => {
	const firstEditor = existingEditors[0]!;
	const originalEditorIndex = group.getIndexOfEditor(firstEditor);
	await group.closeEditor(firstEditor);
	firstEditor.dispose();
	const notebookInput = NotebookEditorInput.create(this.instantiationService, originalInput.resource!, id);
	const newEditor = await group.openEditor(notebookInput, { ...options, index: originalEditorIndex, override: EditorOverride.DISABLED });
	if (newEditor) {
	return newEditor;
	} else {
	return undefined;
	}
	})()
	};
	}
	}
	}
	}
	// Click on the editor tab
	if (id === undefined && originalInput instanceof NotebookEditorInput) {
	const existingEditors = this.editorService.findEditors(originalInput.resource, group).filter(editor => editor instanceof NotebookEditorInput && editor === originalInput);
	if (existingEditors.length) {
	// same
	return undefined;
	}
	}
	if (originalInput instanceof NotebookDiffEditorInput) {
	return undefined;
	}
	let notebookUri: URI = originalInput.resource;
	let cellOptions: IResourceEditorInput | undefined;
	const data = CellUri.parse(originalInput.resource);
	if (data) {
	notebookUri = data.notebook;
	cellOptions = { resource: originalInput.resource, options };
	}
	if (id === undefined && originalInput instanceof ResourceEditorInput) {
	const exitingNotebookEditor = <NotebookEditorInput | undefined>group.editors.find(editor => editor instanceof NotebookEditorInput && isEqual(editor.resource, notebookUri));
	id = exitingNotebookEditor?.viewType;
	}
	if (id === undefined) {
	const existingEditors = this.editorService.findEditors(notebookUri, group).filter(editor =>
	!(editor instanceof NotebookEditorInput)
	&& !(editor instanceof NotebookDiffEditorInput)
	);
	if (existingEditors.length) {
	return undefined;
	}
	const userAssociatedEditors = this.getUserAssociatedEditors(notebookUri);
	const notebookEditor = userAssociatedEditors.filter(association => this.notebookService.getContributedNotebookProvider(association.viewType));
	if (userAssociatedEditors.length && !notebookEditor.length) {
	// user pick a non-notebook editor for this resource
	return undefined;
	}
	// user might pick a notebook editor
	const associatedEditors = distinct([
	...this.getUserAssociatedNotebookEditors(notebookUri),
	...(this.getContributedEditors(notebookUri).filter(editor => editor.priority === NotebookEditorPriority.default))
	], editor => editor.id);
	if (!associatedEditors.length) {
	// there is no notebook editor contribution which is enabled by default
	return undefined;
	}
	}
	const infos = this.notebookService.getContributedNotebookProviders(notebookUri);
	let info = infos.find(info => (!id || info.id === id) && info.exclusive) || infos.find(info => !id || info.id === id);
	if (!info && id !== undefined) {
	info = this.notebookService.getContributedNotebookProvider(id);
	}
	if (!info) {
	return undefined;
	}
	/**
	* Scenario: we are reopening a file editor input which is pinned, we should open in a new editor tab.
	*/
	let index = undefined;
	if (group.activeEditor === originalInput && isEqual(originalInput.resource, notebookUri)) {
	const originalEditorIndex = group.getIndexOfEditor(originalInput);
	index = group.isPinned(originalInput) ? originalEditorIndex + 1 : originalEditorIndex;
	}
	const notebookInput = NotebookEditorInput.create(this.instantiationService, notebookUri, info.id);
	const notebookOptions = new NotebookEditorOptions({ ...options, cellOptions, override: EditorOverride.DISABLED, index });
	return { override: this.editorService.openEditor(notebookInput, notebookOptions, group) };
	}

[jrieken]

tab spying

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

Line 74 in 1c755a4

this._disposables.add(editorService.overrideOpenEditor({

[lramos15]

@jrieken I have fixed the tab spying with an OnWillMoveEditor event. However, the actual opening logic cannot be reduced without adding an intermediary opener that services just custom editors and notebooks as what they do is too specialized to enter into the EditorService.

[lramos15]

This "intermediary" service has been added and upon completion of #121668 this handler will go away all together.

[bpasero]

The handler is gone 👏