Do not store webview if there is no corresponding serializer

[vitaliy-guliy]

Signed-off-by: Vitaliy Gulyy [email protected]

This PR is a duplicate of previously closed #8473

What it does

• Disables saving of the webview state if the plugin don't register an apropriate theia.WebviewPanelSerializer.
• Fixes an error with restoring the editor area when a webview and several editors are opened.

Video demonstrating bug https://www.youtube.com/watch?v=9ir1hTov350

Fixes #8620 and #6877

This is another approach in comparing with #8621
In #8621 Theia stores the empty state for the webview and tries to find the serializer when restoring the state.
In this PR, Theia checks for the theia.WebviewPanelSerializer, and does not store the webview to the layout at all if the serializer is not found.

How to test

To test it you need to have a plugin which opens a webview, but don't register vscode.WebviewPanelSerializer for this webview.
I used this one https://github.com/vitaliy-guliy/greeting-extension, which opens a Greeting webview each time when Theia starts.

Steps to reproduce:

• run Theia with the plugin, open the browser. Greeting plugin will open the webview after startup.
• open few files, Greeting tab must be stay first
• reload the browser

Opened flies must be restored, the order of files must be saved. As the webview is not persisted, after reloading it will be opened again next to active tab.

Follow 'Steps to reproduce' in the original issue #8620

Review checklist

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

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vitaliy-guliy]

@akosyakov what do you think about not to store the webview at all in a case when there is no webview serializer. Instead of saving it with {} inner state and then disposing the widget when restoring the layout.

[tsmaeder]

I think we should handle the case where widgets cannot be recreated. For example, what if a plugin is updated and no longer contributes a webview serializer?
ShellLayoutRestorer#convertToWidget can return undefined and we need to handle that.

[vitaliy-guliy]

I think we should handle the case where widgets cannot be recreated. For example, what if a plugin is updated and no longer contributes a webview serializer?
ShellLayoutRestorer#convertToWidget can return undefined and we need to handle that.

Agree with you. I think thew we need to handle both cases:

• do not store the webview state if there is no serializer
• handle undefined when converting JSON to widget

[vitaliy-guliy]

@tsmaeder done. I've handled a case when the widget cannot be recreated.

[tsmaeder]

T.b.h I liked the previous version better: can't we just filter the array in ShellLayoutRestorer#parse() after we've created all the widgets?

[tsmaeder]

And I like the old version better because the filtering is done closer to where the widget is created (or not created). Simpler to understand when looking at ShellLayoutRestorere.

[vitaliy-guliy]

This what I wanted to do first, but in this case it will not work.
In ShellLayoutRestorer#parse() each widget is created in a separate promise, which is called already after returning array of widgets.

theia/packages/core/src/browser/shell/shell-layout-restorer.ts

Lines 272 to 279 in b54e9eb

	const widgets: (Widget | undefined)[] = [];
	const descs = (value as WidgetDescription[]);
	for (let i = 0; i < descs.length; i++) {
	parseContext.push(async context => {
	widgets[i] = await this.convertToWidget(descs[i], context);
	});
	}
	return widgets;

[tsmaeder]

We wait can for all promises to be fullfilled: we should not return before the widgets array is fully populated anyway:

[vitaliy-guliy]

In previous version we had a problem with order of opened editors. It's because the time for restoring/creating widgets for different files is different and the created widget was always inserted the first.

Layout data is created in ShellLayoutRestorer#inflate function.
At the beginning of this function we parse layout string representation to have LayoutData.

theia/packages/core/src/browser/shell/shell-layout-restorer.ts

Line 228 in b54e9eb

const layout = this.parse<ApplicationShell.LayoutData>(layoutData, parseContext);

But at that moment arrays of widgets are empty for all panels.

At the end we call parseContext.inflate(context). It runs all the promises, which create the widgets and insert them in the array.

theia/packages/core/src/browser/shell/shell-layout-restorer.ts

Line 256 in b54e9eb

await parseContext.inflate(context);

[vitaliy-guliy]

@tsmaeder @spoenemann I have another idea is to move filtering at the end of ShellLayoutRestorer.ParseContext#inflate.

ParseContext has all the arrays to be filtered. So, it will work for all the widget panels.

    export class ParseContext {
        protected readonly toInflate: Inflate[] = [];
        protected readonly toFilter: Widgets[] = [];

        newArray(): Widgets {
            const array: Widgets = [];
            this.toFilter.push(array);
            return array;
        }

        async inflate(context: InflateContext): Promise<void> {
            const pending: Promise<void>[] = [];
            while (this.toInflate.length) {
                pending.push(this.toInflate.pop()!(context));
            }
            await Promise.all(pending);

            if (this.toFilter.length) {
                this.toFilter.forEach(array => {
                    const filtered = array.filter(value => value !== undefined);
                    array.splice(0, array.length, ...filtered);
                });
            }
        }
    }

    export type WidgetArray = (Widget | undefined)[];

Then ShellLayoutRestorer#parse can ask for a widget array, which will be filtered after creating the widgets

    const widgets = parseContext.newArray();
    const descs = (value as WidgetDescription[]);
    for (let i = 0; i < descs.length; i++) {
        parseContext.push(async context => {
            widgets[i] = await this.convertToWidget(descs[i], context);
        });
    }
    return widgets;

I have just checked this approach, it works well.
If it's OK for you, I will update the PR.

[spoenemann]

Sounds good.

[vitaliy-guliy]

Reworked

[tsmaeder]

I really don't understand why this needs to be so complicated: just wait for all the promises to resolve and then filter out the undefined values from the array in ShellLayoutRestorer#parse(). It's better code-quality-wise because it does the filtering closer to where the nulls are created and there is no if conditional expression around the filtering. Please convince me!

[vitaliy-guliy]

It's such confusing because promises for the widgets are created in one place and then resolved in another.
Why do we need so?
It could happen that your Theia is updated or downgraded and the layout cannot be restored properly.
That requires to do some checks for the layout version and probably transform the layout or update the state of some widgets before creating their instances.
Just take a look at ShellLayoutRestorer#inflate()

[azatsarynnyy]

I've tested both use-cases on the provided plug-in https://github.com/vitaliy-guliy/greeting-extension:

• when a contributed WebView has a related registered serializer
• when a contributed WebView without a registered serializer

All widgets restore works well. In both use cases, the behavior is the same as in VS Code 👍
So, I'm +1 for this patch.

It would be great to have another pair of eyes look at it as the proposed changes affect one of the essential part - the application shell.
cc @eclipse-theia/core

[spoenemann]

Performance: Do this splice only if filtered.length is smaller than array.length

[vitaliy-guliy]

Performance: Do this splice only if filtered.length is smaller than array.length

Interesting, I didn't know about it before. In fact the array will not have so much items to work very slow.
But thinking about performance. To avoid checking for an array length, maybe it's better to reset the array at all and then push all the items..

array.length = 0;
array.push(...filtered);

WDYT?

[spoenemann]

That might be an alternative to splice, but checking length for sure has lower cost than resetting the array. And resetting is only necessary if the length has changed.

Another alternative would be this:

for (let i = 0; i < array.length; i++) {
    if (array[i] === undefined) {
        array.splice(i--, 1);
    }
}

If there are only few undefined elements, this variant might be even better because it does not duplicate the array.

[vitaliy-guliy]

In 99% array will not contain undefined elements, so this alternative looks much better than rebuilding the array each time when restoring the layout.
Thanks for this.
I've updated and squashed the PR.

[vitaliy-guliy]

I'm going to merge this PR tomorrow if there are no objections.