From d8c2b56a87ab831a85e24951ec7060910b3b93dc Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Wed, 28 Sep 2022 00:37:48 -0400 Subject: [PATCH] Adds queue for pending notifications Allows for background updates to still notify the graph of changes Removes extra notifications on graph load --- src/plus/webviews/graph/graphWebview.ts | 181 ++++++++++++++++++++++++-------- src/plus/webviews/graph/protocol.ts | 5 + src/webviews/protocol.ts | 2 +- src/webviews/webviewBase.ts | 40 +++++-- 4 files changed, 170 insertions(+), 58 deletions(-) diff --git a/src/plus/webviews/graph/graphWebview.ts b/src/plus/webviews/graph/graphWebview.ts index fb99a57..4e834d2 100644 --- a/src/plus/webviews/graph/graphWebview.ts +++ b/src/plus/webviews/graph/graphWebview.ts @@ -1,4 +1,12 @@ -import type { ColorTheme, ConfigurationChangeEvent, Disposable, Event, StatusBarItem } from 'vscode'; +import type { + ColorTheme, + ConfigurationChangeEvent, + Disposable, + Event, + StatusBarItem, + WebviewOptions, + WebviewPanelOptions, +} from 'vscode'; import { CancellationTokenSource, EventEmitter, MarkdownString, StatusBarAlignment, ViewColumn, window } from 'vscode'; import type { CreatePullRequestActionContext } from '../../../api/gitlens'; import { getAvatarUri } from '../../../avatars'; @@ -40,8 +48,8 @@ import { isDarkTheme, isLightTheme } from '../../../system/utils'; import type { WebviewItemContext } from '../../../system/webview'; import { isWebviewItemContext, serializeWebviewItemContext } from '../../../system/webview'; import { RepositoryFolderNode } from '../../../views/nodes/viewNode'; -import type { IpcMessage } from '../../../webviews/protocol'; import { onIpc } from '../../../webviews/protocol'; +import type { IpcMessage, IpcMessageParams, IpcNotificationType } from '../../../webviews/protocol'; import { WebviewBase } from '../../../webviews/webviewBase'; import type { SubscriptionChangeEvent } from '../../subscription/subscriptionService'; import { ensurePlusFeaturesEnabled } from '../../subscription/utils'; @@ -123,7 +131,7 @@ export class GraphWebview extends WebviewBase { private _etagSubscription?: number; private _etagRepository?: number; private _graph?: GitGraph; - private _pendingNotifyCommits: boolean = false; + private _pendingIpcNotifications = new Map Promise)>(); private _search: GitSearch | undefined; private _searchCancellation: CancellationTokenSource | undefined; private _selectedSha?: string; @@ -148,13 +156,7 @@ export class GraphWebview extends WebviewBase { ); this.disposables.push( configuration.onDidChange(this.onConfigurationChanged, this), - { - dispose: () => { - this._statusBarItem?.dispose(); - void this._repositoryEventsDisposable?.dispose(); - }, - }, - this.container.subscription.onDidChange(this.onSubscriptionChanged, this), + { dispose: () => this._statusBarItem?.dispose() }, registerCommand(Commands.ShowCommitInGraph, (args: ShowCommitInGraphCommandArgs) => { this.repository = this.container.git.getRepository(args.repoPath); this.setSelectedRows(args.sha); @@ -176,6 +178,15 @@ export class GraphWebview extends WebviewBase { this.onConfigurationChanged(); } + protected override get options(): WebviewPanelOptions & WebviewOptions { + return { + retainContextWhenHidden: true, + enableFindWidget: false, + enableCommandUris: true, + enableScripts: true, + }; + } + override async show(options?: { column?: ViewColumn; preserveFocus?: boolean }, ...args: unknown[]): Promise { if (!(await ensurePlusFeaturesEnabled())) return; @@ -259,13 +270,15 @@ export class GraphWebview extends WebviewBase { protected override onInitializing(): Disposable[] | undefined { this._theme = window.activeColorTheme; - return [window.onDidChangeActiveColorTheme(this.onThemeChanged, this)]; + return [ + this.container.subscription.onDidChange(this.onSubscriptionChanged, this), + window.onDidChangeActiveColorTheme(this.onThemeChanged, this), + { dispose: () => void this._repositoryEventsDisposable?.dispose() }, + ]; } protected override onReady(): void { - if (this._pendingNotifyCommits) { - void this.notifyDidChangeCommits(); - } + this.sendPendingIpcNotifications(); } protected override onMessageReceived(e: IpcMessage) { @@ -315,7 +328,10 @@ export class GraphWebview extends WebviewBase { protected override onVisibilityChanged(visible: boolean): void { if (visible && this.repository != null && this.repository.etag !== this._etagRepository) { this.updateState(true); + return; } + + this.sendPendingIpcNotifications(); } private onConfigurationChanged(e?: ConfigurationChangeEvent) { @@ -345,14 +361,17 @@ export class GraphWebview extends WebviewBase { } } - if (configuration.changed(e, 'graph.commitOrdering')) { + // If we don't have an open webview ignore the rest + if (this._panel == null) return; + + if (e != null && configuration.changed(e, 'graph.commitOrdering')) { this.updateState(); return; } if ( - configuration.changed(e, 'defaultDateFormat') || + (e != null && configuration.changed(e, 'defaultDateFormat')) || configuration.changed(e, 'defaultDateStyle') || configuration.changed(e, 'advanced.abbreviatedShaLength') || configuration.changed(e, 'graph.avatars') || @@ -607,7 +626,7 @@ export class GraphWebview extends WebviewBase { @debug() private updateState(immediate: boolean = false) { - if (!this.isReady || !this.visible) return; + this._pendingIpcNotifications.clear(); if (immediate) { void this.notifyDidChangeState(); @@ -625,8 +644,6 @@ export class GraphWebview extends WebviewBase { @debug() private updateAvatars(immediate: boolean = false) { - if (!this.isReady || !this.visible) return; - if (immediate) { void this.notifyDidChangeAvatars(); return; @@ -641,9 +658,9 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeAvatars() { - if (!this.isReady || !this.visible) return false; + if (this._graph == null) return; - const data = this._graph!; + const data = this._graph; return this.notify(DidChangeAvatarsNotificationType, { avatars: Object.fromEntries(data.avatars), }); @@ -651,7 +668,10 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeColumns() { - if (!this.isReady || !this.visible) return false; + if (!this.isReady || !this.visible) { + this.addPendingIpcNotification(DidChangeColumnsNotificationType); + return false; + } const columns = this.getColumns(); return this.notify(DidChangeColumnsNotificationType, { @@ -662,7 +682,10 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeConfiguration() { - if (!this.isReady || !this.visible) return false; + if (!this.isReady || !this.visible) { + this.addPendingIpcNotification(DidChangeGraphConfigurationNotificationType); + return false; + } return this.notify(DidChangeGraphConfigurationNotificationType, { config: this.getComponentConfig(), @@ -671,32 +694,30 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeCommits(completionId?: string) { - let success = false; - - if (this.isReady && this.visible) { - const data = this._graph!; - success = await this.notify( - DidChangeCommitsNotificationType, - { - rows: data.rows, - avatars: Object.fromEntries(data.avatars), - selectedRows: this._selectedRows, - paging: { - startingCursor: data.paging?.startingCursor, - hasMore: data.paging?.hasMore ?? false, - }, - }, - completionId, - ); - } + if (this._graph == null) return; - this._pendingNotifyCommits = !success; - return success; + const data = this._graph; + return this.notify( + DidChangeCommitsNotificationType, + { + rows: data.rows, + avatars: Object.fromEntries(data.avatars), + selectedRows: this._selectedRows, + paging: { + startingCursor: data.paging?.startingCursor, + hasMore: data.paging?.hasMore ?? false, + }, + }, + completionId, + ); } @debug() private async notifyDidChangeSelection() { - if (!this.isReady || !this.visible) return false; + if (!this.isReady || !this.visible) { + this.addPendingIpcNotification(DidChangeSelectionNotificationType); + return false; + } return this.notify(DidChangeSelectionNotificationType, { selection: this._selectedRows, @@ -705,7 +726,10 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeSubscription() { - if (!this.isReady || !this.visible) return false; + if (!this.isReady || !this.visible) { + this.addPendingIpcNotification(DidChangeSubscriptionNotificationType); + return false; + } const access = await this.getGraphAccess(); return this.notify(DidChangeSubscriptionNotificationType, { @@ -716,11 +740,76 @@ export class GraphWebview extends WebviewBase { @debug() private async notifyDidChangeState() { - if (!this.isReady || !this.visible) return false; + if (!this.isReady || !this.visible) { + this.addPendingIpcNotification(DidChangeNotificationType); + return false; + } return this.notify(DidChangeNotificationType, { state: await this.getState() }); } + protected override async notify>( + type: T, + params: IpcMessageParams, + completionId?: string, + ): Promise { + const msg: IpcMessage = { + id: this.nextIpcId(), + method: type.method, + params: params, + completionId: completionId, + }; + const success = await this.postMessage(msg); + if (success) { + this._pendingIpcNotifications.clear(); + } else { + this.addPendingIpcNotification(type, msg); + } + return success; + } + + private readonly _ipcNotificationMap = new Map, () => Promise>([ + [DidChangeColumnsNotificationType, this.notifyDidChangeColumns], + [DidChangeGraphConfigurationNotificationType, this.notifyDidChangeConfiguration], + [DidChangeNotificationType, this.notifyDidChangeState], + [DidChangeSelectionNotificationType, this.notifyDidChangeSelection], + [DidChangeSubscriptionNotificationType, this.notifyDidChangeSubscription], + ]); + + private addPendingIpcNotification(type: IpcNotificationType, msg?: IpcMessage) { + if (type === DidChangeNotificationType) { + this._pendingIpcNotifications.clear(); + } else if (type.overwriteable) { + this._pendingIpcNotifications.delete(type); + } + + let msgOrFn: IpcMessage | (() => Promise) | undefined; + if (msg == null) { + msgOrFn = this._ipcNotificationMap.get(type)?.bind(this); + if (msgOrFn == null) { + debugger; + return; + } + } else { + msgOrFn = msg; + } + this._pendingIpcNotifications.set(type, msgOrFn); + } + + private sendPendingIpcNotifications() { + if (this._pendingIpcNotifications.size === 0) return; + + const ipcs = new Map(this._pendingIpcNotifications); + this._pendingIpcNotifications.clear(); + for (const msgOrFn of ipcs.values()) { + if (typeof msgOrFn === 'function') { + void msgOrFn(); + } else { + void this.postMessage(msgOrFn); + } + } + } + private getColumns(): Record | undefined { return this.container.storage.getWorkspace('graph:columns'); } diff --git a/src/plus/webviews/graph/protocol.ts b/src/plus/webviews/graph/protocol.ts index 66df95d..ae13c4b 100644 --- a/src/plus/webviews/graph/protocol.ts +++ b/src/plus/webviews/graph/protocol.ts @@ -148,6 +148,7 @@ export interface DidChangeGraphConfigurationParams { } export const DidChangeGraphConfigurationNotificationType = new IpcNotificationType( 'graph/configuration/didChange', + true, ); export interface DidChangeSubscriptionParams { @@ -156,6 +157,7 @@ export interface DidChangeSubscriptionParams { } export const DidChangeSubscriptionNotificationType = new IpcNotificationType( 'graph/subscription/didChange', + true, ); export interface DidChangeAvatarsParams { @@ -171,6 +173,7 @@ export interface DidChangeColumnsParams { } export const DidChangeColumnsNotificationType = new IpcNotificationType( 'graph/columns/didChange', + true, ); export interface DidChangeCommitsParams { @@ -188,6 +191,7 @@ export interface DidChangeSelectionParams { } export const DidChangeSelectionNotificationType = new IpcNotificationType( 'graph/selection/didChange', + true, ); export interface DidEnsureCommitParams { @@ -204,4 +208,5 @@ export interface DidSearchCommitsParams { } export const DidSearchCommitsNotificationType = new IpcNotificationType( 'graph/commits/didSearch', + true, ); diff --git a/src/webviews/protocol.ts b/src/webviews/protocol.ts index aad94f4..06d20b0 100644 --- a/src/webviews/protocol.ts +++ b/src/webviews/protocol.ts @@ -9,7 +9,7 @@ export interface IpcMessage { abstract class IpcMessageType { _?: Params; // Required for type inferencing to work properly - constructor(public readonly method: string) {} + constructor(public readonly method: string, public readonly overwriteable: boolean = false) {} } export type IpcMessageParams = T extends IpcMessageType ? P : never; diff --git a/src/webviews/webviewBase.ts b/src/webviews/webviewBase.ts index 2a0827d..56d2f87 100644 --- a/src/webviews/webviewBase.ts +++ b/src/webviews/webviewBase.ts @@ -1,4 +1,10 @@ -import type { Webview, WebviewPanel, WebviewPanelOnDidChangeViewStateEvent } from 'vscode'; +import type { + Webview, + WebviewOptions, + WebviewPanel, + WebviewPanelOnDidChangeViewStateEvent, + WebviewPanelOptions, +} from 'vscode'; import { Disposable, Uri, ViewColumn, window, workspace } from 'vscode'; import { getNonce } from '@env/crypto'; import type { Commands } from '../constants'; @@ -49,6 +55,14 @@ export abstract class WebviewBase implements Disposable { this._disposablePanel?.dispose(); } + protected get options(): WebviewPanelOptions & WebviewOptions { + return { + retainContextWhenHidden: true, + enableFindWidget: true, + enableCommandUris: true, + enableScripts: true, + }; + } private _originalTitle: string | undefined; get originalTitle(): string | undefined { return this._originalTitle; @@ -89,12 +103,7 @@ export abstract class WebviewBase implements Disposable { this.id, this._title, { viewColumn: column, preserveFocus: options?.preserveFocus ?? false }, - { - retainContextWhenHidden: true, - enableFindWidget: true, - enableCommandUris: true, - enableScripts: true, - }, + this.options, ); this._panel.iconPath = Uri.file(this.container.context.asAbsolutePath(this.iconPath)); @@ -249,20 +258,29 @@ export abstract class WebviewBase implements Disposable { return html; } + protected nextIpcId(): string { + return nextIpcId(); + } + protected notify>( type: T, params: IpcMessageParams, completionId?: string, - ): Thenable { - return this.postMessage({ id: nextIpcId(), method: type.method, params: params, completionId: completionId }); + ): Promise { + return this.postMessage({ + id: this.nextIpcId(), + method: type.method, + params: params, + completionId: completionId, + }); } @serialize() @debug['postMessage']>({ args: { 0: m => `(id=${m.id}, method=${m.method}${m.completionId ? `, completionId=${m.completionId}` : ''})` }, }) - private postMessage(message: IpcMessage): Promise { - if (this._panel == null) return Promise.resolve(false); + protected postMessage(message: IpcMessage): Promise { + if (this._panel == null || !this.isReady || !this.visible) return Promise.resolve(false); // It looks like there is a bug where `postMessage` can sometimes just hang infinitely. Not sure why, but ensure we don't hang return Promise.race([