From c4aa2cb81f7789cfc8228443bda41b469d9eba12 Mon Sep 17 00:00:00 2001 From: Bernd Hufmann Date: Tue, 26 Mar 2024 16:56:14 -0400 Subject: [PATCH 1/4] Add default dispose() method to AbstractTraceExplorerProvider Call it from the webview dispose handler. Sub-class can add their own implementation and call super implementation. This avoids that the class is using the webview instance after it is disposed. Signed-off-by: Bernd Hufmann --- .../abstract-trace-explorer-provider.ts | 7 +++- ...plorer-available-views-webview-provider.ts | 23 ++++++------ ...explorer-opened-traces-webview-provider.ts | 11 +++++- ...plorer-time-range-data-webview-provider.ts | 37 +++++++++---------- 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/vscode-trace-extension/src/trace-explorer/abstract-trace-explorer-provider.ts b/vscode-trace-extension/src/trace-explorer/abstract-trace-explorer-provider.ts index c647ba0..5964f59 100644 --- a/vscode-trace-extension/src/trace-explorer/abstract-trace-explorer-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/abstract-trace-explorer-provider.ts @@ -6,7 +6,7 @@ import { traceExtensionWebviewManager } from 'vscode-trace-extension/src/extensi import { VSCODE_MESSAGES } from 'vscode-trace-common/lib/messages/vscode-message-manager'; export abstract class AbstractTraceExplorerProvider implements vscode.WebviewViewProvider { - protected _view: vscode.WebviewView; + protected _view: vscode.WebviewView | undefined; protected _disposables: vscode.Disposable[] = []; /** @@ -62,6 +62,11 @@ export abstract class AbstractTraceExplorerProvider implements vscode.WebviewVie traceExtensionWebviewManager.fireWebviewCreated(webviewView); this.init(webviewView, _context, _token); + webviewView.onDidDispose(_event => this.dispose(), undefined, this._disposables); + } + + protected dispose() { + this._view = undefined; } /* eslint-disable max-len */ diff --git a/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts b/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts index 5fba0e1..e60a6bf 100644 --- a/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts @@ -1,13 +1,13 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import JSONBigConfig from 'json-bigint'; -import * as vscode from 'vscode'; import { signalManager, Signals } from 'traceviewer-base/lib/signals/signal-manager'; import { Experiment } from 'tsp-typescript-client/lib/models/experiment'; import { OutputDescriptor } from 'tsp-typescript-client/lib/models/output-descriptor'; +import * as vscode from 'vscode'; +import { VSCODE_MESSAGES } from 'vscode-trace-common/lib/messages/vscode-message-manager'; +import { convertSignalExperiment } from 'vscode-trace-common/lib/signals/vscode-signal-converter'; import { TraceViewerPanel } from '../../trace-viewer-panel/trace-viewer-webview-panel'; import { getTspClientUrl } from '../../utils/backend-tsp-client-provider'; -import { convertSignalExperiment } from 'vscode-trace-common/lib/signals/vscode-signal-converter'; -import { VSCODE_MESSAGES } from 'vscode-trace-common/lib/messages/vscode-message-manager'; import { AbstractTraceExplorerProvider } from '../abstract-trace-explorer-provider'; const JSONBig = JSONBigConfig({ @@ -40,12 +40,13 @@ export class TraceExplorerAvailableViewsProvider extends AbstractTraceExplorerPr switch (command) { case VSCODE_MESSAGES.CONNECTION_STATUS: if (data && data.status) { - this._statusService.checkAndUpdateServerStatus(); + const status: boolean = JSON.parse(message.data.status); + this._statusService.updateServerStatus(status); } return; case VSCODE_MESSAGES.WEBVIEW_READY: // Post the tspTypescriptClient - this._view.webview.postMessage({ + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.SET_TSP_CLIENT, data: getTspClientUrl() }); @@ -86,13 +87,11 @@ export class TraceExplorerAvailableViewsProvider extends AbstractTraceExplorerPr ); signalManager().on(Signals.EXPERIMENT_SELECTED, this._onExperimentSelected); - webviewView.onDidDispose( - _event => { - signalManager().off(Signals.EXPERIMENT_SELECTED, this._onExperimentSelected); - }, - undefined, - this._disposables - ); + } + + protected dispose() { + signalManager().off(Signals.EXPERIMENT_SELECTED, this._onExperimentSelected); + super.dispose(); } private _onExperimentSelected = (experiment: Experiment | undefined): void => diff --git a/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts b/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts index de7b7d7..b9e72b3 100644 --- a/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts @@ -66,12 +66,13 @@ export class TraceExplorerOpenedTracesViewProvider extends AbstractTraceExplorer switch (command) { case VSCODE_MESSAGES.CONNECTION_STATUS: if (data && data.status) { - this._statusService.checkAndUpdateServerStatus(); + const status: boolean = JSON.parse(message.data.status); + this._statusService.updateServerStatus(status); } return; case VSCODE_MESSAGES.WEBVIEW_READY: // Post the tspTypescriptClient - this._view.webview.postMessage({ + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.SET_TSP_CLIENT, data: getTspClientUrl() }); @@ -137,4 +138,10 @@ export class TraceExplorerOpenedTracesViewProvider extends AbstractTraceExplorer this._disposables ); } + protected dispose() { + signalManager().off(Signals.TRACEVIEWERTAB_ACTIVATED, this._onOpenedTracesWidgetActivated); + signalManager().off(Signals.EXPERIMENT_SELECTED, this._onExperimentSelected); + signalManager().off(Signals.EXPERIMENT_OPENED, this._onExperimentOpened); + super.dispose(); + } } diff --git a/vscode-trace-extension/src/trace-explorer/time-range/trace-explorer-time-range-data-webview-provider.ts b/vscode-trace-extension/src/trace-explorer/time-range/trace-explorer-time-range-data-webview-provider.ts index 3995da0..c28c08b 100644 --- a/vscode-trace-extension/src/trace-explorer/time-range/trace-explorer-time-range-data-webview-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/time-range/trace-explorer-time-range-data-webview-provider.ts @@ -41,12 +41,12 @@ export class TraceExplorerTimeRangeDataProvider extends AbstractTraceExplorerPro ); webviewView.onDidChangeVisibility(() => { - if (this._view.visible) { + if (this._view?.visible) { const data = { mapArray: Array.from(this._experimentDataMap.experimentDataMap.values()), activeData: this._experimentDataMap.activeData }; - this._view.webview.postMessage({ + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.RESTORE_VIEW, data: JSONBig.stringify(data) }); @@ -59,23 +59,20 @@ export class TraceExplorerTimeRangeDataProvider extends AbstractTraceExplorerPro signalManager().on(Signals.EXPERIMENT_UPDATED, this.onExperimentUpdated); signalManager().on(Signals.EXPERIMENT_CLOSED, this.onExperimentClosed); signalManager().on(Signals.CLOSE_TRACEVIEWERTAB, this.onExperimentTabClosed); + } - webviewView.onDidDispose( - _event => { - signalManager().off(Signals.VIEW_RANGE_UPDATED, this.onViewRangeUpdated); - signalManager().off(Signals.SELECTION_RANGE_UPDATED, this.onSelectionRangeUpdated); - signalManager().off(Signals.EXPERIMENT_SELECTED, this.onExperimentSelected); - signalManager().off(Signals.EXPERIMENT_UPDATED, this.onExperimentUpdated); - signalManager().off(Signals.EXPERIMENT_CLOSED, this.onExperimentClosed); - signalManager().off(Signals.CLOSE_TRACEVIEWERTAB, this.onExperimentTabClosed); - }, - undefined, - this._disposables - ); + protected dispose() { + signalManager().off(Signals.VIEW_RANGE_UPDATED, this.onViewRangeUpdated); + signalManager().off(Signals.SELECTION_RANGE_UPDATED, this.onSelectionRangeUpdated); + signalManager().off(Signals.EXPERIMENT_SELECTED, this.onExperimentSelected); + signalManager().off(Signals.EXPERIMENT_UPDATED, this.onExperimentUpdated); + signalManager().off(Signals.EXPERIMENT_CLOSED, this.onExperimentClosed); + signalManager().off(Signals.CLOSE_TRACEVIEWERTAB, this.onExperimentTabClosed); + super.dispose(); } private onViewRangeUpdated = (update: TimeRangeUpdatePayload) => { - this._view.webview.postMessage({ + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.VIEW_RANGE_UPDATED, data: JSONBig.stringify(update) }); @@ -83,7 +80,7 @@ export class TraceExplorerTimeRangeDataProvider extends AbstractTraceExplorerPro }; private onSelectionRangeUpdated = (update: TimeRangeUpdatePayload) => { - this._view.webview.postMessage({ + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.SELECTION_RANGE_UPDATED, data: JSONBig.stringify(update) }); @@ -92,7 +89,7 @@ export class TraceExplorerTimeRangeDataProvider extends AbstractTraceExplorerPro private onExperimentSelected = (experiment: Experiment | undefined) => { const data = { wrapper: experiment ? JSONBig.stringify(experiment) : undefined }; - this._view.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_SELECTED, data }); + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_SELECTED, data }); if (experiment) { this._experimentDataMap.updateAbsoluteRange(experiment); } @@ -101,18 +98,18 @@ export class TraceExplorerTimeRangeDataProvider extends AbstractTraceExplorerPro private onExperimentUpdated = (experiment: Experiment) => { const data = { wrapper: JSONBig.stringify(experiment) }; - this._view.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_UPDATED, data }); + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_UPDATED, data }); this._experimentDataMap.updateAbsoluteRange(experiment); }; private onExperimentClosed = (experiment: Experiment) => { const data = { wrapper: JSONBig.stringify(experiment) }; - this._view.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_CLOSED, data }); + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.EXPERIMENT_CLOSED, data }); this._experimentDataMap.delete(experiment); }; private onExperimentTabClosed = (experimentUUID: string) => { - this._view.webview.postMessage({ command: VSCODE_MESSAGES.TRACE_VIEWER_TAB_CLOSED, data: experimentUUID }); + this._view?.webview.postMessage({ command: VSCODE_MESSAGES.TRACE_VIEWER_TAB_CLOSED, data: experimentUUID }); this._experimentDataMap.delete(experimentUUID); }; } From e4ecdb3d9f07327ff35188227af78be4f7eb0ece Mon Sep 17 00:00:00 2001 From: Bernd Hufmann Date: Tue, 26 Mar 2024 16:58:39 -0400 Subject: [PATCH 2/4] Ignore connection status listener notification in constructor The TSP client implementation notifies clients when the method addConnectionStatusListener() is called. This can lead to incorrect state in on client side. Signed-off-by: Bernd Hufmann --- .../src/client/tsp-client-provider-impl.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/vscode-trace-common/src/client/tsp-client-provider-impl.ts b/vscode-trace-common/src/client/tsp-client-provider-impl.ts index f80db2c..9c0f837 100644 --- a/vscode-trace-common/src/client/tsp-client-provider-impl.ts +++ b/vscode-trace-common/src/client/tsp-client-provider-impl.ts @@ -10,13 +10,21 @@ export class TspClientProvider implements ITspClientProvider { private _traceManager: TraceManager; private _experimentManager: ExperimentManager; private _listeners: ((tspClient: TspClient) => void)[] = []; + private _initialized = false; constructor( private _url: string, private _signalHandler: VsCodeMessageManager | undefined ) { this.updateClients(); - RestClient.addConnectionStatusListener(status => this._signalHandler?.notifyConnection(status)); + + RestClient.addConnectionStatusListener(status => { + // Ignore the first update that is sent when calling addConnectionStatusListener + if (this._initialized) { + this._signalHandler?.notifyConnection(status); + } + }); + this._initialized = true; this._tspClient.checkHealth(); // When this is called in the remote use-case, it will block the port-forwarding service-worker. } From b63b236cd75278b639a38c7ca145f340918988e7 Mon Sep 17 00:00:00 2001 From: Bernd Hufmann Date: Tue, 26 Mar 2024 17:16:30 -0400 Subject: [PATCH 3/4] Add method to set server status in TraceServerStatusService This method set the status bar and the serverUp context that is used to decide whether the trace explorer or the welcome view should be rendered. This method doesn't check the backend to get status in comparison to checkAndUpdateServerStatus. It's useful when it's known that the server is down or up (e.g. after start notification). In this case, it is not necessary to check the backend again. Don't set the status in the method isTraceServerUp() because it should not have side-effects that are not expected. Initialize and refresh flags traceViewer.serverUp and trace-explorer.noExperiments to ensure the correct view is rendered, i.e. Trace Explorer or Welcome view, when certain event happen (e.g. server started or stopped). Fixes #227 Fixes #228 Signed-off-by: Bernd Hufmann --- vscode-trace-extension/src/extension.ts | 25 +++++++++++++------ ...plorer-available-views-webview-provider.ts | 2 +- ...explorer-opened-traces-webview-provider.ts | 2 +- .../trace-viewer-webview-panel.ts | 3 ++- .../src/utils/trace-server-status.ts | 8 +++++- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/vscode-trace-extension/src/extension.ts b/vscode-trace-extension/src/extension.ts index ec739c8..6b93491 100644 --- a/vscode-trace-extension/src/extension.ts +++ b/vscode-trace-extension/src/extension.ts @@ -80,7 +80,7 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI { await startTraceServerIfAvailable(file.fsPath); if (await isTraceServerUp()) { fileOpenHandler(context, file); - vscode.commands.executeCommand('setContext', 'trace-explorer.noExperiments', false); + vscode.commands.executeCommand('trace-explorer.refreshContext'); } }) ); @@ -181,6 +181,7 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI { await startTraceServerIfAvailable(traceUri.fsPath); if (await isTraceServerUp()) { fileOpenHandler(context, traceUri); + serverStatusService.updateServerStatus(true); vscode.commands.executeCommand('setContext', 'trace-explorer.noExperiments', false); } }) @@ -193,24 +194,28 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI { ); context.subscriptions.push( - vscode.commands.registerCommand('serverStatus.started', () => { - serverStatusService.checkAndUpdateServerStatus(); + vscode.commands.registerCommand('serverStatus.started', async () => { + await serverStatusService.updateServerStatus(true); if (tracesProvider) { + // Trigger webview refresh tracesProvider.postMessagetoWebview(VSCODE_MESSAGES.TRACE_SERVER_STARTED, undefined); } + // Refresh so that either trace explorer or welcome page is rendered + updateNoExperimentsContext(); }) ); context.subscriptions.push( - vscode.commands.registerCommand('serverStatus.stopped', () => { - serverStatusService.checkAndUpdateServerStatus(); + vscode.commands.registerCommand('serverStatus.stopped', async () => { + await serverStatusService.updateServerStatus(false); }) ); context.subscriptions.push( vscode.commands.registerCommand('trace-explorer.refreshContext', async () => { + // Refresh so that either trace explorer or welcome page is rendered const isUp = await isTraceServerUp(); - vscode.commands.executeCommand('setContext', 'traceViewer.serverUp', isUp); + await serverStatusService.updateServerStatus(isUp); if (isUp) { await updateNoExperimentsContext(); } @@ -219,9 +224,13 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI { vscode.commands.executeCommand('setContext', 'traceViewer.markerSetsPresent', false); vscode.commands.executeCommand('setContext', 'traceViewer.markerCategoriesPresent', false); - vscode.commands.executeCommand('setContext', 'trace-explorer.noExperiments', true); + + // Initialize noExperiments/serverUp in a way so that trace explorer webviews are initialized + vscode.commands.executeCommand('setContext', 'trace-explorer.noExperiments', false); + vscode.commands.executeCommand('setContext', 'traceViewer.serverUp', true); + + // Refresh to trigger rendering trace explorer or welcome page vscode.commands.executeCommand('trace-explorer.refreshContext'); - serverStatusService.checkAndUpdateServerStatus(); return traceExtensionAPI; } diff --git a/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts b/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts index e60a6bf..5e283cd 100644 --- a/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts @@ -39,7 +39,7 @@ export class TraceExplorerAvailableViewsProvider extends AbstractTraceExplorerPr const data: any = message.data; switch (command) { case VSCODE_MESSAGES.CONNECTION_STATUS: - if (data && data.status) { + if (data?.status) { const status: boolean = JSON.parse(message.data.status); this._statusService.updateServerStatus(status); } diff --git a/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts b/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts index b9e72b3..69c795f 100644 --- a/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts +++ b/vscode-trace-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts @@ -65,7 +65,7 @@ export class TraceExplorerOpenedTracesViewProvider extends AbstractTraceExplorer const data: any = message.data; switch (command) { case VSCODE_MESSAGES.CONNECTION_STATUS: - if (data && data.status) { + if (data?.status) { const status: boolean = JSON.parse(message.data.status); this._statusService.updateServerStatus(status); } diff --git a/vscode-trace-extension/src/trace-viewer-panel/trace-viewer-webview-panel.ts b/vscode-trace-extension/src/trace-viewer-panel/trace-viewer-webview-panel.ts index 7a4c298..cc835db 100644 --- a/vscode-trace-extension/src/trace-viewer-panel/trace-viewer-webview-panel.ts +++ b/vscode-trace-extension/src/trace-viewer-panel/trace-viewer-webview-panel.ts @@ -255,7 +255,8 @@ export class TraceViewerPanel { return; case VSCODE_MESSAGES.CONNECTION_STATUS: if (message.data?.status && this._statusService) { - this._statusService.checkAndUpdateServerStatus(); + const status: boolean = JSON.parse(message.data.status); + this._statusService.updateServerStatus(status); } return; case VSCODE_MESSAGES.SHOW_MARKER_CATEGORIES: diff --git a/vscode-trace-extension/src/utils/trace-server-status.ts b/vscode-trace-extension/src/utils/trace-server-status.ts index 9f1dfc2..8c3e53f 100644 --- a/vscode-trace-extension/src/utils/trace-server-status.ts +++ b/vscode-trace-extension/src/utils/trace-server-status.ts @@ -1,5 +1,6 @@ import { ThemeColor, StatusBarItem } from 'vscode'; import { isTraceServerUp } from './backend-tsp-client-provider'; +import * as vscode from 'vscode'; export class TraceServerConnectionStatusService { private statusBarItem: StatusBarItem; @@ -11,9 +12,14 @@ export class TraceServerConnectionStatusService { public checkAndUpdateServerStatus = async (): Promise => { const isUp = await isTraceServerUp(); - this.render(isUp); + await this.updateServerStatus(isUp); }; + public async updateServerStatus(status: boolean): Promise { + await vscode.commands.executeCommand('setContext', 'traceViewer.serverUp', status); + this.render(status); + } + private render = (status: boolean): void => { if (status) { this.statusBarItem.backgroundColor = new ThemeColor('statusBarItem.warningBackground'); From 1f6b33a067153c8ae0455b291e6c7555f789ca07 Mon Sep 17 00:00:00 2001 From: Bernd Hufmann Date: Tue, 26 Mar 2024 17:20:02 -0400 Subject: [PATCH 4/4] Refresh trace viewer after server URL preference changes For the case that with current URL the welcome view is rendered due to noExperiment or server is down, this will render the trace explorer (or welcome view) after a server URL change. Without calling refresh, the welcome view stays, even if the other server has experiments. Fixes #229 Signed-off-by: Bernd Hufmann --- vscode-trace-common/src/client/tsp-client-provider-impl.ts | 7 ++++--- vscode-trace-extension/src/extension.ts | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/vscode-trace-common/src/client/tsp-client-provider-impl.ts b/vscode-trace-common/src/client/tsp-client-provider-impl.ts index 9c0f837..04105a8 100644 --- a/vscode-trace-common/src/client/tsp-client-provider-impl.ts +++ b/vscode-trace-common/src/client/tsp-client-provider-impl.ts @@ -20,11 +20,12 @@ export class TspClientProvider implements ITspClientProvider { RestClient.addConnectionStatusListener(status => { // Ignore the first update that is sent when calling addConnectionStatusListener - if (this._initialized) { - this._signalHandler?.notifyConnection(status); + if (!this._initialized) { + this._initialized = true; + return; } + this._signalHandler?.notifyConnection(status); }); - this._initialized = true; this._tspClient.checkHealth(); // When this is called in the remote use-case, it will block the port-forwarding service-worker. } diff --git a/vscode-trace-extension/src/extension.ts b/vscode-trace-extension/src/extension.ts index 6b93491..c059871 100644 --- a/vscode-trace-extension/src/extension.ts +++ b/vscode-trace-extension/src/extension.ts @@ -106,6 +106,9 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI { // Signal the change to all trace panels TraceViewerPanel.updateTraceServerUrl(newTspClientURL); + + // Refresh so that either trace explorer or welcome page is rendered + vscode.commands.executeCommand('trace-explorer.refreshContext'); } }) );