Fix the widget options when opening a VSCode Custom Editor

[debovema]

What it does

Fixes #10583

How to test

See #10583

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

[colin-grant-work]

@debovema, thanks for your contribution! I believe the code is good, but should be moved elsewhere. See below.

It looks like this is should be the responsibility of the open handler for the widget. For example, here's the default WidgetOpenHandler:

theia/packages/core/src/browser/widget-open-handler.ts

Lines 98 to 100 in 17b419f

	if (!widget.isAttached) {
	this.shell.addWidget(widget, op.widgetOptions || { area: 'main' });
	}

But the same is not done when a new custom editor is created:

theia/packages/plugin-ext/src/main/browser/custom-editors/custom-editor-opener.tsx

Lines 83 to 87 in 17b419f

	widget = await widgetPromise;
	this.pendingWidgetPromises.delete(uriString);
	widget.viewType = this.editor.viewType;
	widget.resource = uri;
	this.onDidOpenCustomEditorEmitter.fire(widget);

I think that for consistency's sake, that's the place to handle this issue.

[colin-grant-work]

Alternatively, it looks like this is where the attachment is handled, at the moment:

theia/packages/plugin-ext/src/main/browser/custom-editors/custom-editors-main.ts

Lines 234 to 240 in 17b419f

	if (view.isAttached) {
	if (view.isVisible) {
	this.shell.revealWidget(view.id);
	}
	return;
	}
	this.webviewsMain.addOrReattachWidget(view, { preserveFocus: true });

[debovema]

Hi @colin-grant-work

Thank you for your review.

The issue with your suggestion to move the logic in the Custom Editor Opener (custom-editor-opener.tsx) is that we don't have the info provided by VSCode extension being opened in this location.

The other place in $createCustomEditorPanel method in custom-editors-main.ts does receive a viewColumn parameter but AFAICS this parameter is hardcoded to EditorPosition.ONE, because of this:

theia/packages/plugin-ext/src/main/browser/custom-editors/custom-editors-main.ts

Line 152 in 17b419f

EditorPosition.ONE, // TODO: fix this when Theia has support splitting editors,

If I understand well, the call to await openHandler.open(uri, editorOptions); actually delegates to the CustomEditorOpener class (this class is not injectable with inversify so I assume it is created here :

theia/packages/plugin-ext/src/main/browser/custom-editors/plugin-custom-editor-registry.ts

Line 84 in 17b419f

const editorOpenHandler = new CustomEditorOpener(

So it might be the simplest solution to add the behaviour in my original place, don't you think ?

[colin-grant-work]

The issue with your suggestion to move the logic in the Custom Editor Opener (custom-editor-opener.tsx) is that we don't have the info provided by VSCode extension being opened in this location.

I'm not sure what you mean by this. As you say

the call to await openHandler.open(uri, editorOptions); actually delegates to the CustomEditorOpener

the call to openHandler.open calls into CustomEditorOpener.open, so if we have the information we need where you've added it, then we have that very same information in custom-editor-opener.tsx, because we're passing that information to the CustomEditorOpener, and we have access to the ApplicationShell there, as well, so there's nothing in the code you've added that we couldn't do in the CustomEditorOpener class.

Regarding the hardcoded EditorPosition.ONE and the comment there, I don't think I understand what they mean. Placing a widget in a particular column doesn't necessarily have anything to do with whether the widget can be split, but I haven't worked much with custom editors, myself, so there may be some logic there that I'm missing. To my mind, the problem has nothing to do with the splittability of editors but rather with whether Theia correctly handles the viewColumn field, and there is code that appears to try to do so:

theia/packages/plugin-ext/src/main/browser/webviews-main.ts

Lines 108 to 114 in d7cdb85

	} else if (showOptions.viewColumn >= 0) {
	const columnsSize = this.viewColumnService.viewColumnsSize();
	if (columnsSize) {
	showOptions.viewColumn = columnsSize - 1;
	widgetIds = this.viewColumnService.getViewColumnIds(showOptions.viewColumn);
	}
	}

So it may be worth simply swapping out that hardcoded value with whatever is contextually appropriate to see if that addresses the problem.

[debovema]

the call to openHandler.open calls into CustomEditorOpener.open, so if we have the information we need where you've added it, then we have that very same information in custom-editor-opener.tsx, because we're passing that information to the CustomEditorOpener, and we have access to the ApplicationShell there, as well, so there's nothing in the code you've added that we couldn't do in the CustomEditorOpener class.

I pushed another commit moving the logic in the CustomEditorOpener.

Please notice that the options parameter has been changed from OpenerOptions to WidgetOpenerOptions.
I hope there are no impact elsewhere.

The general behaviour is fine but there are some errors in the console worth investigating :

[debovema]

@colin-grant-work : eventually I fully reworked the propagation of WidgetOpenerOptions from CustomEditorOpener to CustomEditorsMainImpl

[colin-grant-work]

@debovema,

@colin-grant-work : eventually I fully reworked the propagation of WidgetOpenerOptions from CustomEditorOpener to CustomEditorsMainImpl

Does the latest code cover functionality apart from what's described in #10583, or do the testing steps remain the same?

[debovema]

No it covers the exact same original issue.
But the implementation is more consistent.

[colin-grant-work]

The other thing to consider here is that VSCode opener options, which is what is expected from plugins, differ from Theia's opener options, so there may be opposition to applying the Theia logic to a case where it is not expected to apply. It should at least be avoided that the Theia logic is applied when no widgetOptions field is present: in that case, the existing handler should be used.

[colin-grant-work]

The new code looks good to me and handles options passed by the plugin consistently and correctly.

[debovema]

Hi,

Is this PR in the waiting line for being merged ?

[msujew]

@debovema We sometimes wait for a few days before merging to get input from any willing other reviewers as well :)

Anyway, this looks good to me as well, so merge at your leisure @colin-grant-work 👍

[debovema]

Thank you @msujew for the follow-up and @colin-grant-work for the review !
👍 🎉