Improve implementation around widget management (#13818)

Mainly, ensures that any widget promises returned by the `WidgetManager` will only resolve when the entire widget creation process has completed successfully, including the 'onWillCreateWidget` part.
eclipse-theia · Jun 27, 2024 · f996098 · f996098
1 parent fe70ec5
commit f996098
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 14 deletions.
diff --git a/packages/core/src/browser/widget-manager.ts b/packages/core/src/browser/widget-manager.ts
@@ -114,7 +114,7 @@ export class WidgetManager {
 
     protected _cachedFactories: Map<string, WidgetFactory>;
     protected readonly widgets = new Map<string, Widget>();
-    protected readonly pendingWidgetPromises = new Map<string, MaybePromise<Widget>>();
+    protected readonly pendingWidgetPromises = new Map<string, Promise<Widget>>();
 
     @inject(ContributionProvider) @named(WidgetFactory)
     protected readonly factoryProvider: ContributionProvider<WidgetFactory>;
@@ -207,9 +207,9 @@ export class WidgetManager {
                 return widget as T;
             }
         }
-        for (const [key, widget] of this.pendingWidgetPromises.entries()) {
+        for (const [key, widgetPromise] of this.pendingWidgetPromises.entries()) {
             if (this.testPredicate(key, factoryId, predicate)) {
-                return widget as T;
+                return widgetPromise as Promise<T>;
             }
         }
     }
@@ -244,18 +244,26 @@ export class WidgetManager {
         if (!factory) {
             throw Error("No widget factory '" + factoryId + "' has been registered.");
         }
-        try {
-            const widgetPromise = factory.createWidget(options);
-            this.pendingWidgetPromises.set(key, widgetPromise);
-            const widget = await widgetPromise;
-            await WaitUntilEvent.fire(this.onWillCreateWidgetEmitter, { factoryId, widget });
+        const widgetPromise = this.doCreateWidget<T>(factory, options).then(widget => {
             this.widgets.set(key, widget);
             widget.disposed.connect(() => this.widgets.delete(key));
             this.onDidCreateWidgetEmitter.fire({ factoryId, widget });
-            return widget as T;
-        } finally {
-            this.pendingWidgetPromises.delete(key);
+            return widget;
+        }).finally(() => this.pendingWidgetPromises.delete(key));
+        this.pendingWidgetPromises.set(key, widgetPromise);
+        return widgetPromise;
+    }
+
+    protected async doCreateWidget<T extends Widget>(factory: WidgetFactory, options?: any): Promise<T> {
+        const widget = await factory.createWidget(options);
+        // Note: the widget creation process also includes the 'onWillCreateWidget' part, which can potentially fail
+        try {
+            await WaitUntilEvent.fire(this.onWillCreateWidgetEmitter, { factoryId: factory.id, widget });
+        } catch (e) {
+            widget.dispose();
+            throw e;
         }
+        return widget as T;
     }
 
     /**

diff --git a/packages/core/src/browser/widget-open-handler.ts b/packages/core/src/browser/widget-open-handler.ts
@@ -96,7 +96,7 @@ export abstract class WidgetOpenHandler<W extends BaseWidget> implements OpenHan
             ...options
         };
         if (!widget.isAttached) {
-            this.shell.addWidget(widget, op.widgetOptions || { area: 'main' });
+            await this.shell.addWidget(widget, op.widgetOptions || { area: 'main' });
         }
         if (op.mode === 'activate') {
             await this.shell.activateWidget(widget.id);
@@ -143,12 +143,12 @@ export abstract class WidgetOpenHandler<W extends BaseWidget> implements OpenHan
 
     protected getWidget(uri: URI, options?: WidgetOpenerOptions): Promise<W | undefined> {
         const widgetOptions = this.createWidgetOptions(uri, options);
-        return this.widgetManager.getWidget<W>(this.id, widgetOptions);
+        return this.widgetManager.getWidget(this.id, widgetOptions);
     }
 
     protected getOrCreateWidget(uri: URI, options?: WidgetOpenerOptions): Promise<W> {
         const widgetOptions = this.createWidgetOptions(uri, options);
-        return this.widgetManager.getOrCreateWidget<W>(this.id, widgetOptions);
+        return this.widgetManager.getOrCreateWidget(this.id, widgetOptions);
     }
 
     protected abstract createWidgetOptions(uri: URI, options?: WidgetOpenerOptions): Object;