From 9a19f1cb475b66f1f4ff74c7cd72817e43c3697f Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Thu, 4 Jun 2020 12:36:34 +0000 Subject: [PATCH] [plugin] ensure that views are prepared before activating them Signed-off-by: Anton Kosyakov --- CHANGELOG.md | 3 ++ .../src/browser/shell/application-shell.ts | 28 +++++++-------- packages/core/src/browser/widget-manager.ts | 28 ++++++++++++--- .../src/browser/editor-preview-widget.ts | 5 ++- .../main/browser/view/plugin-view-registry.ts | 35 +++++++++++-------- 5 files changed, 63 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a3309df6d32c..bb5bb5c533449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,11 @@ # Change Log +## v1.3.0 + Breaking Changes: - [task] Widened the scope of some methods in TaskManager and TaskConfigurations from string to TaskConfigurationScope. This is only breaking for extenders, not callers. [#7928](https://github.com/eclipse-theia/theia/pull/7928) +- [shell] `ApplicationShell.TrackableWidgetProvider.getTrackableWidgets` is sync to register child widgets in the same tick, use `ApplicationShell.TrackableWidgetProvider.onDidChangeTrackableWidgets` if child widgets are added async ## v1.2.0 diff --git a/packages/core/src/browser/shell/application-shell.ts b/packages/core/src/browser/shell/application-shell.ts index 0db8b9b83cea8..89105291f7b69 100644 --- a/packages/core/src/browser/shell/application-shell.ts +++ b/packages/core/src/browser/shell/application-shell.ts @@ -23,7 +23,7 @@ import { } from '@phosphor/widgets'; import { Message } from '@phosphor/messaging'; import { IDragEvent } from '@phosphor/dragdrop'; -import { RecursivePartial, MaybePromise, Event as CommonEvent, DisposableCollection, Disposable } from '../../common'; +import { RecursivePartial, Event as CommonEvent, DisposableCollection, Disposable } from '../../common'; import { animationFrame } from '../browser'; import { Saveable, SaveableWidget } from '../saveable'; import { StatusBarImpl, StatusBarEntry, StatusBarAlignment } from '../status-bar/status-bar'; @@ -610,18 +610,18 @@ export class ApplicationShell extends Widget { const { mainPanel, bottomPanel, leftPanel, rightPanel, activeWidgetId } = layoutData; if (leftPanel) { this.leftPanelHandler.setLayoutData(leftPanel); - await this.registerWithFocusTracker(leftPanel); + this.registerWithFocusTracker(leftPanel); } if (rightPanel) { this.rightPanelHandler.setLayoutData(rightPanel); - await this.registerWithFocusTracker(rightPanel); + this.registerWithFocusTracker(rightPanel); } // Proceed with the bottom panel once the side panels are set up await Promise.all([this.leftPanelHandler.state.pendingUpdate, this.rightPanelHandler.state.pendingUpdate]); if (bottomPanel) { if (bottomPanel.config) { this.bottomPanel.restoreLayout(bottomPanel.config); - await this.registerWithFocusTracker(bottomPanel.config.main); + this.registerWithFocusTracker(bottomPanel.config.main); } if (bottomPanel.size) { this.bottomPanelState.lastPanelSize = bottomPanel.size; @@ -637,7 +637,7 @@ export class ApplicationShell extends Widget { await this.bottomPanelState.pendingUpdate; if (mainPanel) { this.mainPanel.restoreLayout(mainPanel); - await this.registerWithFocusTracker(mainPanel.main); + this.registerWithFocusTracker(mainPanel.main); } if (activeWidgetId) { this.activateWidget(activeWidgetId); @@ -679,22 +679,22 @@ export class ApplicationShell extends Widget { /** * Track all widgets that are referenced by the given layout data. */ - protected async registerWithFocusTracker(data: DockLayout.ITabAreaConfig | DockLayout.ISplitAreaConfig | SidePanel.LayoutData | null): Promise { + protected registerWithFocusTracker(data: DockLayout.ITabAreaConfig | DockLayout.ISplitAreaConfig | SidePanel.LayoutData | null): void { if (data) { if (data.type === 'tab-area') { for (const widget of data.widgets) { if (widget) { - await this.track(widget); + this.track(widget); } } } else if (data.type === 'split-area') { for (const child of data.children) { - await this.registerWithFocusTracker(child); + this.registerWithFocusTracker(child); } } else if (data.type === 'sidepanel' && data.items) { for (const item of data.items) { if (item.widget) { - await this.track(item.widget); + this.track(item.widget); } } } @@ -759,7 +759,7 @@ export class ApplicationShell extends Widget { throw new Error('Unexpected area: ' + options.area); } if (area !== 'top') { - await this.track(widget); + this.track(widget); } } @@ -958,7 +958,7 @@ export class ApplicationShell extends Widget { /** * Track the given widget so it is considered in the `current` and `active` state of the shell. */ - protected async track(widget: Widget): Promise { + protected track(widget: Widget): void { if (this.tracker.widgets.indexOf(widget) !== -1) { return; } @@ -966,8 +966,8 @@ export class ApplicationShell extends Widget { this.checkActivation(widget); Saveable.apply(widget); if (ApplicationShell.TrackableWidgetProvider.is(widget)) { - for (const toTrack of await widget.getTrackableWidgets()) { - await this.track(toTrack); + for (const toTrack of widget.getTrackableWidgets()) { + this.track(toTrack); } if (widget.onDidChangeTrackableWidgets) { widget.onDidChangeTrackableWidgets(widgets => widgets.forEach(w => this.track(w))); @@ -1860,7 +1860,7 @@ export namespace ApplicationShell { * Exposes widgets which activation state should be tracked by shell. */ export interface TrackableWidgetProvider { - getTrackableWidgets(): MaybePromise + getTrackableWidgets(): Widget[] readonly onDidChangeTrackableWidgets?: CommonEvent /** * Make visible and focus a trackable widget for the given id. diff --git a/packages/core/src/browser/widget-manager.ts b/packages/core/src/browser/widget-manager.ts index 4d2f343f8bcba..161514aec47a9 100644 --- a/packages/core/src/browser/widget-manager.ts +++ b/packages/core/src/browser/widget-manager.ts @@ -16,7 +16,7 @@ import { inject, named, injectable } from 'inversify'; import { Widget } from '@phosphor/widgets'; -import { ILogger, Emitter, Event, ContributionProvider, MaybePromise } from '../common'; +import { ILogger, Emitter, Event, ContributionProvider, MaybePromise, WaitUntilEvent } from '../common'; /* eslint-disable @typescript-eslint/no-explicit-any */ export const WidgetFactory = Symbol('WidgetFactory'); @@ -53,6 +53,20 @@ export interface WidgetConstructionOptions { options?: any } +/** + * Representation of a `willCreateWidgetEvent`. + */ +export interface WillCreateWidgetEvent extends WaitUntilEvent { + /** + * The widget which will be created. + */ + readonly widget: Widget; + /** + * The widget factory id. + */ + readonly factoryId: string; +} + /** * Representation of a `didCreateWidgetEvent`. */ @@ -84,6 +98,13 @@ export class WidgetManager { @inject(ILogger) protected readonly logger: ILogger; + protected readonly onWillCreateWidgetEmitter = new Emitter(); + /** + * An event can be used to participate in the widget creation. + * Listeners may not dispose the given widget. + */ + readonly onWillCreateWidget: Event = this.onWillCreateWidgetEmitter.event; + protected readonly onDidCreateWidgetEmitter = new Emitter(); readonly onDidCreateWidget: Event = this.onDidCreateWidgetEmitter.event; @@ -154,15 +175,14 @@ export class WidgetManager { const widgetPromise = factory.createWidget(options); this.pendingWidgetPromises.set(key, widgetPromise); const widget = await widgetPromise; + await WaitUntilEvent.fire(this.onWillCreateWidgetEmitter, { factoryId, widget }); this.widgetPromises.set(key, widgetPromise); this.widgets.set(key, widget); widget.disposed.connect(() => { this.widgets.delete(key); this.widgetPromises.delete(key); }); - this.onDidCreateWidgetEmitter.fire({ - factoryId, widget - }); + this.onDidCreateWidgetEmitter.fire({ factoryId, widget }); return widget as T; } finally { this.pendingWidgetPromises.delete(key); diff --git a/packages/editor-preview/src/browser/editor-preview-widget.ts b/packages/editor-preview/src/browser/editor-preview-widget.ts index 4ed16ad2fc98a..05f682fd03509 100644 --- a/packages/editor-preview/src/browser/editor-preview-widget.ts +++ b/packages/editor-preview/src/browser/editor-preview-widget.ts @@ -198,9 +198,8 @@ export class EditorPreviewWidget extends BaseWidget implements ApplicationShell. } } - getTrackableWidgets(): Promise { - return new Promise( - resolve => resolve(this.editorWidget_ ? [this.editorWidget_] : [])); + getTrackableWidgets(): Widget[] { + return this.editorWidget_ ? [this.editorWidget_] : []; } storeState(): PreviewViewState { diff --git a/packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts b/packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts index 64b896d62911d..b14294ed45912 100644 --- a/packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts +++ b/packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts @@ -114,22 +114,22 @@ export class PluginViewRegistry implements FrontendApplicationContribution { this.updateFocusedView(); this.shell.onDidChangeActiveWidget(() => this.updateFocusedView()); - this.widgetManager.onDidCreateWidget(({ factoryId, widget }) => { + this.widgetManager.onWillCreateWidget(({ factoryId, widget, waitUntil }) => { if (factoryId === EXPLORER_VIEW_CONTAINER_ID && widget instanceof ViewContainerWidget) { - this.prepareViewContainer('explorer', widget); + waitUntil(this.prepareViewContainer('explorer', widget)); } if (factoryId === SCM_VIEW_CONTAINER_ID && widget instanceof ViewContainerWidget) { - this.prepareViewContainer('scm', widget); + waitUntil(this.prepareViewContainer('scm', widget)); } if (factoryId === DebugWidget.ID && widget instanceof DebugWidget) { const viewContainer = widget['sessionWidget']['viewContainer']; - this.prepareViewContainer('debug', viewContainer); + waitUntil(this.prepareViewContainer('debug', viewContainer)); } if (factoryId === PLUGIN_VIEW_CONTAINER_FACTORY_ID && widget instanceof ViewContainerWidget) { - this.prepareViewContainer(this.toViewContainerId(widget.options), widget); + waitUntil(this.prepareViewContainer(this.toViewContainerId(widget.options), widget)); } if (factoryId === PLUGIN_VIEW_FACTORY_ID && widget instanceof PluginViewWidget) { - this.prepareView(widget); + waitUntil(this.prepareView(widget)); } }); this.doRegisterViewContainer('test', 'left', { @@ -212,18 +212,18 @@ export class PluginViewRegistry implements FrontendApplicationContribution { id: toggleCommandId, label: 'Toggle ' + options.label + ' View' }, { - execute: async () => { - let widget = await this.getPluginViewContainer(id); + execute: async () => { + let widget = await this.getPluginViewContainer(id); + if (widget) { + widget.dispose(); + } else { + widget = await this.openViewContainer(id); if (widget) { - widget.dispose(); - } else { - widget = await this.openViewContainer(id); - if (widget) { - this.shell.activateWidget(widget.id); - } + this.shell.activateWidget(widget.id); } } - })); + } + })); toDispose.push(this.menus.registerMenuAction(CommonMenus.VIEW_VIEWS, { commandId: toggleCommandId, label: options.label @@ -309,6 +309,11 @@ export class PluginViewRegistry implements FrontendApplicationContribution { widget.title.label = view.name; const currentDataWidget = widget.widgets[0]; const viewDataWidget = await this.createViewDataWidget(view.id); + if (widget.isDisposed) { + // eslint-disable-next-line no-unused-expressions + viewDataWidget?.dispose(); + return; + } if (currentDataWidget !== viewDataWidget) { if (currentDataWidget) { currentDataWidget.dispose();