Revisit handling of deleted/moved files in custom editors

[bpasero]

I just came across the change in b52f1c7#diff-bd0cac707b6707e6ef725a11d0702cea. I think the approach is a bit questionable as we now have some editors implementing this method (custom) but some not (files).

I find the FileEditorTracker should only be responsible for updating editors for files, not any other editor nor custom editors. At least that was the initial intent of it.

Can you do the tracking on your end for custom editors? I am not seeing a big win of doing this in a generic way?

[bpasero]

I pushed changes to revert back to how it was. I have a feeling that each editor should handle moves and deletes individually, specifically I think CustomEditorService should listen to file operations and update accordingly (via this._register(this.fileService.onAfterOperation(e => this.onFileOperation(e)))).

The reason I feel this cannot be handled in a generic way is that file changes and file operations are very specific and not something every editor should care about. So a method such as EditorInput.handleMove or handleDelete does not make a lot of sense to me when called from file changes. The fact that custom editors need to listen on file changes is imho an implementation detail.

Let's put that on the agenda for our sync.

[bpasero]

I briefly looked into this but don't have a good understanding of how this should work. Specifically:

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

Lines 161 to 174 in e8b79d9

	public handleMove(groupId: GroupIdentifier, uri: URI, options?: ITextEditorOptions): IEditorInput | undefined {
	const editorInfo = this.customEditorService.getCustomEditor(this.viewType);
	if (editorInfo?.matches(uri)) {
	const webview = assertIsDefined(this.takeOwnershipOfWebview());
	const newInput = this.instantiationService.createInstance(CustomFileEditorInput,
	uri,
	this.viewType,
	generateUuid(),
	new Lazy(() => webview));
	newInput.updateGroup(groupId);
	return newInput;
	}
	return undefined;
	}

It looks like we used to pass in the new URI after the rename into editorInfo?.matches(uri) but that seems wrong to me. Shouldn't we find the custom editor that deals with the resource before it got renamed and then update the editor accordingly?

Is there a way to trigger a custom editor on a file resource to try to reproduce?

[bpasero]

@mjbvz I spend some time working on it via 7293979 and went back to a solution that would expose a move method from the editor input, sorry for the back and forth.

It has the following signature:

move(group: GroupIdentifier, target: URI): IMoveResult | undefined

export interface IMoveResult {
	editor: IEditorInput | IResourceEditor;
	options?: IEditorOptions;
}

The idea is that you can return a new editor input that you would like to be replaced with and provide some additional options if e.g. you want to restore specific view state once the editor is being replaced.

I was not adopting this for custom editor input because I was not sure how to create the input from within the custom input, maybe you could have a look and then delete your custom logic for handling moves.

Btw deletes are also handled now, a file that is deleted that matches an editor that is opened will dispose that editor. But I think you have extra logic already somewhere to dispose webviews when files are deleted, maybe you could check.

[mjbvz]

Thanks @bpasero! Just got around to taking another look at this with 041a590

I'm going to continue looking into how VS Code and extensions handle moves in #86146