Merge editor and editor resolver

[bpasero]

This is somewhat related to #153110 and also #153340: today the new merge editor is not registering itself to the IEditorResolverService, except for that hack to enable opening the merge editor ad-hoc:

vscode/src/vs/workbench/contrib/mergeEditor/browser/view/mergeEditor.ts

Line 412 in 71b996e

this._editorOverrideHandle.value = this._editorResolverService.registerEditor(

This brings up a more general question: how would the merge editor register to the resolver, ideally I would like to do run this code to open a merge editor:

const mergeEditor = { input1: {resource: URI}, input2: {resource: URI}, base: { resource: URI}};
editorService.openEditor(mergeEditor);

This follows our pattern of untyped editor inputs for opening.

We have similar support for opening diff editors via the untyped IResourceDiffEditorInput:

vscode/src/vs/workbench/common/editor.ts

Lines 474 to 485 in 71b996e

	export interface IResourceDiffEditorInput extends IBaseUntypedEditorInput {
	/**
	* The left hand side editor to open inside a diff editor.
	*/
	readonly original: IResourceEditorInput | ITextResourceEditorInput | IUntitledTextResourceEditorInput;
	/**
	* The right hand side editor to open inside a diff editor.
	*/
	readonly modified: IResourceEditorInput | ITextResourceEditorInput | IUntitledTextResourceEditorInput;
	}

Following the factory method of the resolver, I think we almost need to introduce a new factory method createMergeEditorInput, specifically for merge editors, which would then also theoretically enable us to do a merge editor for notebooks:

vscode/src/vs/workbench/services/editor/common/editorResolverService.ts

Lines 142 to 149 in 71b996e

	registerEditor(
	globPattern: string | glob.IRelativePattern,
	editorInfo: RegisteredEditorInfo,
	options: RegisteredEditorOptions,
	createEditorInput: EditorInputFactoryFunction,
	createUntitledEditorInput?: UntitledEditorInputFactoryFunction | undefined,
	createDiffEditorInput?: DiffEditorInputFactoryFunction
	): IDisposable;

Btw: Untyped editor inputs enable a few nice features, such as dragging an editor and dropping it into a new window to "move" it there. Since we cannot serialize a typed editor input to the target window, we have to use the untyped variant. This works by calling the method toUntyped on a typed editor input (e.g. here for file inputs):

[jrieken]

how would the merge editor register to the resolver, ideally I would like to do run this code to open a merge editor:

IDK - that means we push the notion of merge editors into workbench as a built-in concept. I am fine with that but IMO we are already paying much for abstractions and registrations (registerEditorPane, registerEditorSerializer, input to editor mapping etc...) and I really wonder what they are for then?

Since we cannot serialize a typed editor input to the target window, we have to use the untyped variant. This works by calling the method toUntyped on a typed editor input (e.g. here for file inputs):

@bpasero Why isn't this using the IEditorSerializer-concept?

[bpasero]

and I really wonder what they are for then

They are for creating an editor out of a { resource: URI } lightweight input. For example, this enables clicking a file in the explorer to open a notebook and not a text file. Or even open a custom editor (for example binary editor).

We do not have to introduce the notion of merge editors if we can encode a merge editor input as a single resource, which is doable, but is a bit inconsistent with how diff editors work. Maybe Logan has an idea here how to proceed.

Why isn't this using the IEditorSerializer-concept

Will check and see what the reasons are.

[lramos15]

We do not have to introduce the notion of merge editors if we can encode a merge editor input as a single resource, which is doable, but is a bit inconsistent with how diff editors work. Maybe Logan has an idea here how to proceed.

We thought about doing this for diff for the tabs API but it sounds messy. I think adding createMergeEditorInput factory is the best way to proceed, there wouldn't be a new registration in this case though as the text registration would just have its factory called when three inputs are passed in via the appropriate untyped editor. There are tons of benefits to untype editors that make this worthwhile especially as we deprecate typed editors more and more. This should also easily enable other three-way merge scenarios as @bpasero has pointed out

[jrieken]

I think adding createMergeEditorInput factory is the best way to proceed,

Where would that happen - in the workbench or contrib?

[lramos15]

Where would that happen - in the workbench or contrib?
We would add it to the register editor type and I would integrate it in such that we choose to create that editor in the case of an untyped merge editor input. Here's we would add the factory function

vscode/src/vs/workbench/services/textfile/common/textEditorService.ts

Lines 69 to 81 in a72212f

	this._register(this.editorResolverService.registerEditor(
	'*',
	{
	id: DEFAULT_EDITOR_ASSOCIATION.id,
	label: DEFAULT_EDITOR_ASSOCIATION.displayName,
	detail: DEFAULT_EDITOR_ASSOCIATION.providerDisplayName,
	priority: RegisteredEditorPriority.builtin
	},
	{},
	editor => ({ editor: this.createTextEditor(editor) }),
	untitledEditor => ({ editor: this.createTextEditor(untitledEditor) }),
	diffEditor => ({ editor: this.createTextEditor(diffEditor) })
	));

[jrieken]

So, that would mean we cannot be a contribution anymore and move into the core, right? I not really sure I see the grand scheme of things and why we need to register an editor in the first place. I know, I have done it for some very special in-merge-editor-code-navigation-logic (#153110) but it is not that we want to open a merge editor from the explorer or so

[lramos15]

So, that would mean we cannot be a contribution anymore and move into the core, right? I not really sure I see the grand scheme of things and why we need to register an editor in the first place. I know, I have done it for some very special in-merge-editor-code-navigation-logic (#153110) but it is not that we want to open a merge editor from the explorer or so

You could keep it as a contrib if merge editors have a special schema that you would register instead. The reason I suggested it here as that would allow any arbitrary three resources to be opened in a three way merge. Registration also would enable vscode.open and in the future the tabs API open calls to function properly since they resolve on taking a lightweight untyped input passing it through the resolver and getting a fully fledged EditorInput out in the end

[bpasero]

The only reason merge editor would move into core is because text editors are registered in core and the merge editor is a text editor. We always had text editors in workbench core and never in contrib. That does not mean that the editor resolver is not fullfilling its purpose, we could as well move all text editors out into contrib if we wanted to.

The editor resolver abstracts underlying implementation by introducing the notion of untyped inputs. It does so by exposing the intent of an editor, but not its implementation: normal editor, diff editor, merge editor, untitled editor. I think having a merge editor as another kind is good, because - as I had said before - once we have a merge editor for notebooks, we could allow resolving a merge conflict in a dedicated notebook merge editor if the file is a notebook.

When I look at the sources of the merge editor, I think the following structure could work:

• vs/workbench/contrib/mergeEditor/common/mergeEditor.ts renames to vs/workbench/common/editor/mergeEditor.ts
• vs/workbench/contrib/mergeEditor/browser renames to vs/workbench/browser/parts/editor/mergeEditor

[bpasero]

This somewhat blocks #5770 because CLI support requires some generic untyped input to open on startup or into an existing window. If 👍 I can maybe work with Logan to get this in, but it would need the suggested move to core unless the editor resolver could provide a way to "enrich" an existing contribution with another capability (merge editors):

vscode/src/vs/workbench/services/textfile/common/textEditorService.ts

Lines 68 to 82 in e69176a

	private registerDefaultEditor(): void {
	this._register(this.editorResolverService.registerEditor(
	'*',
	{
	id: DEFAULT_EDITOR_ASSOCIATION.id,
	label: DEFAULT_EDITOR_ASSOCIATION.displayName,
	detail: DEFAULT_EDITOR_ASSOCIATION.providerDisplayName,
	priority: RegisteredEditorPriority.builtin
	},
	{},
	editor => ({ editor: this.createTextEditor(editor) }),
	untitledEditor => ({ editor: this.createTextEditor(untitledEditor) }),
	diffEditor => ({ editor: this.createTextEditor(diffEditor) })
	));
	}

Not sure whether that would be possible Logan?

[jrieken]

Let's set up a sync to talk about this first. I am not really understanding the whole picture yet (e.g I wonder if notebooks, assuming they will have a merge editor, also move to core)

[bpasero]

From our meeting:

• introduce IUntypedMergeEditorInput: { base: URI; remote: URI; local: URI; result: URI } in core
• add new method on editor resolver registration: createMergeEditor
• allow multiple registrations on the same glob and pick the one that provides the right editor class (normal, diff, untitled, merge)
• change the editor registration to an object to avoid repeated undefined
• check if we then can remove the redundant canHandleDiff option

Some follow up ideas:

• rename untyped input to unresolved input

[bpasero]

This was reverted and will re-land in August, @lramos15 can you recreate the PR?

[lramos15]

Recreated #156672 will keep as draft until I can ensure merge is working and the perf issues are ironed out