Refactor extension context subscriptions

Use DiposableObject more consistently and ensure all commands are added as a disposable to the ExtensionContext.
2020-10-12 14:59:00 -07:00
parent 2dcf3b3feb
commit fe219e05d8
5 changed files with 90 additions and 68 deletions
--- a/extensions/ql-vscode/src/astViewer.ts
+++ b/extensions/ql-vscode/src/astViewer.ts
@@ -1,6 +1,5 @@
 import {
  window,
-  ExtensionContext,
  TreeDataProvider,
  EventEmitter,
  Event,
@@ -19,6 +18,7 @@ import { UrlValue, BqrsId } from './bqrs-cli-types';
 import { showLocation } from './interface-utils';
 import { isStringLoc, isWholeFileLoc, isLineColumnLoc } from './bqrs-utils';
 import { commandRunner } from './helpers';
+import { DisposableObject } from './vscode-utils/disposable-object';

 export interface AstItem {
  id: BqrsId;
@@ -33,7 +33,7 @@ export interface ChildAstItem extends AstItem {
  parent: ChildAstItem | AstItem;
 }

-class AstViewerDataProvider implements TreeDataProvider<AstItem> {
+class AstViewerDataProvider  extends DisposableObject implements TreeDataProvider<AstItem> {

  public roots: AstItem[] = [];
  public db: DatabaseItem | undefined;
@@ -44,10 +44,13 @@ class AstViewerDataProvider implements TreeDataProvider<AstItem> {
    this._onDidChangeTreeData.event;

  constructor() {
-    commandRunner('codeQLAstViewer.gotoCode',
-    async (item: AstItem) => {
-      await showLocation(item.fileLocation);
-    });
+    super();
+    this.push(
+      commandRunner('codeQLAstViewer.gotoCode',
+      async (item: AstItem) => {
+        await showLocation(item.fileLocation);
+      })
+    );
  }

  refresh(): void {
@@ -96,23 +99,28 @@ class AstViewerDataProvider implements TreeDataProvider<AstItem> {
  }
 }

-export class AstViewer {
+export class AstViewer extends DisposableObject {
  private treeView: TreeView<AstItem>;
  private treeDataProvider: AstViewerDataProvider;
  private currentFile: string | undefined;

-  constructor(ctx: ExtensionContext) {
+  constructor() {
+    super();
+
    this.treeDataProvider = new AstViewerDataProvider();
    this.treeView = window.createTreeView('codeQLAstViewer', {
      treeDataProvider: this.treeDataProvider,
      showCollapseAll: true
    });

-    commandRunner('codeQLAstViewer.clear', async () => {
-      this.clear();
-    });
-
-    ctx.subscriptions.push(window.onDidChangeTextEditorSelection(this.updateTreeSelection, this));
+    this.push(this.treeView);
+    this.push(this.treeDataProvider);
+    this.push(
+      commandRunner('codeQLAstViewer.clear', async () => {
+        this.clear();
+      })
+    );
+    this.push(window.onDidChangeTextEditorSelection(this.updateTreeSelection, this));
  }

  updateRoots(roots: AstItem[], db: DatabaseItem, fileName: string) {
--- a/extensions/ql-vscode/src/databases-ui.ts
+++ b/extensions/ql-vscode/src/databases-ui.ts
@@ -3,7 +3,6 @@ import { DisposableObject } from './vscode-utils/disposable-object';
 import {
  Event,
  EventEmitter,
-  ExtensionContext,
  ProviderResult,
  TreeDataProvider,
  TreeItem,
@@ -83,8 +82,8 @@ class DatabaseTreeDataProvider extends DisposableObject
  private currentDatabaseItem: DatabaseItem | undefined;

  constructor(
-    private ctx: ExtensionContext,
-    private databaseManager: DatabaseManager
+    private databaseManager: DatabaseManager,
+    private readonly extensionPath: string
  ) {
    super();

@@ -131,12 +130,12 @@ class DatabaseTreeDataProvider extends DisposableObject
    const item = new TreeItem(element.name);
    if (element === this.currentDatabaseItem) {
      item.iconPath = joinThemableIconPath(
-        this.ctx.extensionPath,
+        this.extensionPath,
        SELECTED_DATABASE_ICON
      );
    } else if (element.error !== undefined) {
      item.iconPath = joinThemableIconPath(
-        this.ctx.extensionPath,
+        this.extensionPath,
        INVALID_DATABASE_ICON
      );
    }
@@ -213,16 +212,16 @@ export class DatabaseUI extends DisposableObject {
  private treeDataProvider: DatabaseTreeDataProvider;

  public constructor(
-    ctx: ExtensionContext,
    private cliserver: cli.CodeQLCliServer,
    private databaseManager: DatabaseManager,
    private readonly queryServer: qsClient.QueryServerClient | undefined,
-    private readonly storagePath: string
+    private readonly storagePath: string,
+    readonly extensionPath: string
  ) {
    super();

    this.treeDataProvider = this.push(
-      new DatabaseTreeDataProvider(ctx, databaseManager)
+      new DatabaseTreeDataProvider(databaseManager, extensionPath)
    );
    this.push(
      window.createTreeView('codeQLDatabases', {
@@ -232,7 +231,7 @@ export class DatabaseUI extends DisposableObject {
    );

    logger.log('Registering database panel commands.');
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQL.setCurrentDatabase',
        this.handleSetCurrentDatabase,
@@ -241,7 +240,7 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQL.upgradeCurrentDatabase',
        this.handleUpgradeCurrentDatabase,
@@ -251,7 +250,7 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQL.clearCache',
        this.handleClearCache,
@@ -260,7 +259,7 @@ export class DatabaseUI extends DisposableObject {
        })
    );

-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQLDatabases.chooseDatabaseFolder',
        this.handleChooseDatabaseFolder,
@@ -269,7 +268,7 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQLDatabases.chooseDatabaseArchive',
        this.handleChooseDatabaseArchive,
@@ -278,7 +277,7 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQLDatabases.chooseDatabaseInternet',
        this.handleChooseDatabaseInternet,
@@ -287,7 +286,7 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQLDatabases.chooseDatabaseLgtm',
        this.handleChooseDatabaseLgtm,
@@ -295,31 +294,31 @@ export class DatabaseUI extends DisposableObject {
          title: 'Adding database from LGTM',
        })
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.setCurrentDatabase',
        this.handleMakeCurrentDatabase
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.sortByName',
        this.handleSortByName
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.sortByDateAdded',
        this.handleSortByDateAdded
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.removeDatabase',
        this.handleRemoveDatabase
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunnerWithProgress(
        'codeQLDatabases.upgradeDatabase',
        this.handleUpgradeDatabase,
@@ -329,13 +328,13 @@ export class DatabaseUI extends DisposableObject {
        }
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.renameDatabase',
        this.handleRenameDatabase
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      commandRunner(
        'codeQLDatabases.openDatabaseFolder',
        this.handleOpenFolder
--- a/extensions/ql-vscode/src/extension.ts
+++ b/extensions/ql-vscode/src/extension.ts
@@ -338,11 +338,11 @@ async function activateWithInstalledDistribution(
  ctx.subscriptions.push(dbm);
  logger.log('Initializing database panel.');
  const databaseUI = new DatabaseUI(
-    ctx,
    cliServer,
    dbm,
    qs,
-    getContextStoragePath(ctx)
+    getContextStoragePath(ctx),
+    ctx.extensionPath
  );
  ctx.subscriptions.push(databaseUI);

@@ -352,13 +352,14 @@ async function activateWithInstalledDistribution(
    showResultsForCompletedQuery(item, WebviewReveal.Forced);

  const qhm = new QueryHistoryManager(
-    ctx,
    qs,
+    ctx.extensionPath,
    queryHistoryConfigurationListener,
    showResults,
    async (from: CompletedQuery, to: CompletedQuery) =>
      showResultsForComparison(from, to),
  );
+  ctx.subscriptions.push(qhm);
  logger.log('Initializing results panel interface.');
  const intm = new InterfaceManager(ctx, dbm, cliServer, queryServerLogger);
  ctx.subscriptions.push(intm);
@@ -621,7 +622,8 @@ async function activateWithInstalledDistribution(
    new TemplateQueryReferenceProvider(cliServer, qs, dbm)
  );

-  const astViewer = new AstViewer(ctx);
+  const astViewer = new AstViewer();
+  ctx.subscriptions.push(astViewer);
  ctx.subscriptions.push(helpers.commandRunnerWithProgress('codeQL.viewAst', async (
    progress: helpers.ProgressCallback,
    token: CancellationToken
--- a/extensions/ql-vscode/src/query-history.ts
+++ b/extensions/ql-vscode/src/query-history.ts
@@ -1,6 +1,6 @@
 import * as path from 'path';
 import * as vscode from 'vscode';
-import { ExtensionContext, window as Window } from 'vscode';
+import { window as Window } from 'vscode';
 import { CompletedQuery } from './query-results';
 import { QueryHistoryConfig } from './config';
 import { QueryWithResults } from './run-queries';
@@ -8,6 +8,7 @@ import * as helpers from './helpers';
 import { logger } from './logging';
 import { URLSearchParams } from 'url';
 import { QueryServerClient } from './queryserver-client';
+import { DisposableObject } from './vscode-utils/disposable-object';

 /**
 * query-history.ts
@@ -73,12 +74,19 @@ class HistoryTreeDataProvider implements QueryHistoryDataProvider {

  private history: CompletedQuery[] = [];

+  private failedIconPath: string;
+
  /**
   * When not undefined, must be reference-equal to an item in `this.databases`.
   */
  private current: CompletedQuery | undefined;

-  constructor(private ctx: ExtensionContext) { }
+  constructor(extensionPath: string) {
+    this.failedIconPath = path.join(
+      extensionPath,
+      FAILED_QUERY_HISTORY_ITEM_ICON
+    );
+  }

  async updateTreeItemContextValue(element: CompletedQuery): Promise<void> {
    // Mark this query history item according to whether it has a
@@ -107,10 +115,7 @@ class HistoryTreeDataProvider implements QueryHistoryDataProvider {
    this.updateTreeItemContextValue(element);

    if (!element.didRunSuccessfully) {
-      it.iconPath = path.join(
-        this.ctx.extensionPath,
-        FAILED_QUERY_HISTORY_ITEM_ICON
-      );
+      it.iconPath = this.failedIconPath;
    }

    return it;
@@ -173,15 +178,15 @@ class HistoryTreeDataProvider implements QueryHistoryDataProvider {
 */
 const DOUBLE_CLICK_TIME = 500;

-export class QueryHistoryManager {
+export class QueryHistoryManager extends DisposableObject {
  treeDataProvider: HistoryTreeDataProvider;
  treeView: vscode.TreeView<CompletedQuery>;
  lastItemClick: { time: Date; item: CompletedQuery } | undefined;
  compareWithItem: CompletedQuery | undefined;

  constructor(
-    ctx: ExtensionContext,
    private qs: QueryServerClient,
+    extensionPath: string,
    private queryHistoryConfigListener: QueryHistoryConfig,
    private selectedCallback: (item: CompletedQuery) => Promise<void>,
    private doCompareCallback: (
@@ -189,76 +194,84 @@ export class QueryHistoryManager {
      to: CompletedQuery
    ) => Promise<void>
  ) {
+    super();
+
    const treeDataProvider = (this.treeDataProvider = new HistoryTreeDataProvider(
-      ctx
+      extensionPath
    ));
    this.treeView = Window.createTreeView('codeQLQueryHistory', {
      treeDataProvider,
      canSelectMany: true,
    });
+    this.push(this.treeView);

    // Lazily update the tree view selection due to limitations of TreeView API (see
    // `updateTreeViewSelectionIfVisible` doc for details)
-    this.treeView.onDidChangeVisibility(async (_ev) =>
-      this.updateTreeViewSelectionIfVisible()
+    this.push(
+      this.treeView.onDidChangeVisibility(async (_ev) =>
+        this.updateTreeViewSelectionIfVisible()
+      )
    );
    // Don't allow the selection to become empty
-    this.treeView.onDidChangeSelection(async (ev) => {
-      if (ev.selection.length == 0) {
-        this.updateTreeViewSelectionIfVisible();
-      }
-      this.updateCompareWith(ev.selection);
-    });
+    this.push(
+      this.treeView.onDidChangeSelection(async (ev) => {
+        if (ev.selection.length == 0) {
+          this.updateTreeViewSelectionIfVisible();
+        }
+        this.updateCompareWith(ev.selection);
+      })
+    );
+
    logger.log('Registering query history panel commands.');
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.openQuery',
        this.handleOpenQuery.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.removeHistoryItem',
        this.handleRemoveHistoryItem.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.setLabel',
        this.handleSetLabel.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.compareWith',
        this.handleCompareWith.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.showQueryLog',
        this.handleShowQueryLog.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.showQueryText',
        this.handleShowQueryText.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.viewSarif',
        this.handleViewSarif.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.viewDil',
        this.handleViewDil.bind(this)
      )
    );
-    ctx.subscriptions.push(
+    this.push(
      helpers.commandRunner(
        'codeQLQueryHistory.itemClicked',
        async (item: CompletedQuery) => {
--- a/extensions/ql-vscode/src/vscode-tests/no-workspace/astViewer.test.ts
+++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/astViewer.test.ts
@@ -5,7 +5,7 @@ import * as sinon from 'sinon';
 import * as yaml from 'js-yaml';

 import { AstViewer, AstItem } from '../../astViewer';
-import { ExtensionContext, commands, Range } from 'vscode';
+import { commands, Range } from 'vscode';
 import { DatabaseItem } from '../../databases';

 chai.use(chaiAsPromised);
@@ -32,7 +32,7 @@ describe('AstViewer', () => {

  it('should update the viewer roots', () => {
    const item = {} as DatabaseItem;
-    viewer = new AstViewer({ subscriptions: [] as any[] } as ExtensionContext);
+    viewer = new AstViewer();
    viewer.updateRoots(astRoots, item, 'def/abc');

    expect((viewer as any).treeDataProvider.roots).to.eq(astRoots);
@@ -63,7 +63,7 @@ describe('AstViewer', () => {
    fsPath = 'def/abc',
  ) {
    const item = {} as DatabaseItem;
-    viewer = new AstViewer({ subscriptions: [] as any[] } as ExtensionContext);
+    viewer = new AstViewer();
    viewer.updateRoots(astRoots, item, fsPath);
    const spy = sinon.spy();
    (viewer as any).treeView.reveal = spy;