Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve implementation around widget management #13818

Merged
merged 1 commit into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions packages/core/src/browser/widget-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down Expand Up @@ -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>;
}
}
}
Expand Down Expand Up @@ -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;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/browser/widget-open-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

}
if (op.mode === 'activate') {
await this.shell.activateWidget(widget.id);
Expand Down Expand Up @@ -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;
Expand Down
Loading