From 1f7d0fa09b0c4a531d1c19a431ef6fd1858fd617 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 15 Dec 2020 15:33:19 -0800 Subject: [PATCH 1/2] Fix set label command on history items This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes #598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation. --- extensions/ql-vscode/CHANGELOG.md | 1 + extensions/ql-vscode/src/extension.ts | 2 +- extensions/ql-vscode/src/query-history.ts | 51 ++++++++--------------- extensions/ql-vscode/src/query-results.ts | 3 +- 4 files changed, 21 insertions(+), 36 deletions(-) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index e941f9d1350..b07e4b7f49e 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -11,6 +11,7 @@ - Fix a bug where it is not possible to download some database archives. This fix specifically addresses large archives and archives whose central directories do not align with file headers. [#700](https://github.com/github/vscode-codeql/pull/700) - Avoid error dialogs when QL test discovery or database cleanup encounters a missing directory. [#706](https://github.com/github/vscode-codeql/pull/706) - Add descriptive text and a link in the results view. [#711](https://github.com/github/vscode-codeql/pull/711) +- Fix the _Set Label_ command in the query history view. [#710](https://github.com/github/vscode-codeql/pull/710) ## 1.3.7 - 24 November 2020 diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index a5339c170c4..a2b4db4628a 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -452,7 +452,7 @@ async function activateWithInstalledDistribution( // The call to showResults potentially creates SARIF file; // Update the tree item context value to allow viewing that // SARIF file from context menu. - await qhm.updateTreeItemContextValue(item); + await qhm.refreshTreeView(item); } } diff --git a/extensions/ql-vscode/src/query-history.ts b/extensions/ql-vscode/src/query-history.ts index 36a0f0d0f64..fbdbe82c934 100644 --- a/extensions/ql-vscode/src/query-history.ts +++ b/extensions/ql-vscode/src/query-history.ts @@ -52,14 +52,10 @@ const SHOW_QUERY_TEXT_QUICK_EVAL_MSG = `\ */ const FAILED_QUERY_HISTORY_ITEM_ICON = 'media/red-x.svg'; -interface QueryHistoryDataProvider extends vscode.TreeDataProvider { - updateTreeItemContextValue(element: CompletedQuery): Promise; -} - /** * Tree data provider for the query history view. */ -class HistoryTreeDataProvider extends DisposableObject implements QueryHistoryDataProvider { +class HistoryTreeDataProvider extends DisposableObject { private _onDidChangeTreeData = super.push(new vscode.EventEmitter()); readonly onDidChangeTreeData: vscode.Event = this @@ -82,37 +78,28 @@ class HistoryTreeDataProvider extends DisposableObject implements QueryHistoryDa ); } - async updateTreeItemContextValue(element: CompletedQuery): Promise { - // Mark this query history item according to whether it has a - // SARIF file so that we can make context menu items conditionally - // available. - const hasResults = await element.query.hasInterpretedResults(); - element.treeItem!.contextValue = hasResults - ? 'interpretedResultsItem' - : 'rawResultsItem'; - this.refresh(); - } - async getTreeItem(element: CompletedQuery): Promise { - if (element.treeItem !== undefined) - return element.treeItem; - - const it = new vscode.TreeItem(element.toString()); + const treeItem = new vscode.TreeItem(element.toString()); - it.command = { + treeItem.command = { title: 'Query History Item', command: 'codeQLQueryHistory.itemClicked', arguments: [element], }; - element.treeItem = it; - this.updateTreeItemContextValue(element); + // Mark this query history item according to whether it has a + // SARIF file so that we can make context menu items conditionally + // available. + const hasResults = await element.query.hasInterpretedResults(); + treeItem.contextValue = hasResults + ? 'interpretedResultsItem' + : 'rawResultsItem'; if (!element.didRunSuccessfully) { - it.iconPath = this.failedIconPath; + treeItem.iconPath = this.failedIconPath; } - return it; + return treeItem; } getChildren( @@ -157,8 +144,8 @@ class HistoryTreeDataProvider extends DisposableObject implements QueryHistoryDa return this.history; } - refresh() { - this._onDidChangeTreeData.fire(undefined); + refresh(CompletedQuery?: CompletedQuery) { + this._onDidChangeTreeData.fire(CompletedQuery); } find(queryId: number): CompletedQuery | undefined { @@ -367,10 +354,8 @@ export class QueryHistoryManager extends DisposableObject { }); // undefined response means the user cancelled the dialog; don't change anything if (response !== undefined) { - if (response === '') - // Interpret empty string response as 'go back to using default' - singleItem.options.label = undefined; - else singleItem.options.label = response; + // Interpret empty string response as 'go back to using default' + singleItem.options.label = response === '' ? undefined : response; this.treeDataProvider.refresh(); } } @@ -695,7 +680,7 @@ the file in the file explorer and dragging it into the workspace.` }; } - async updateTreeItemContextValue(element: CompletedQuery): Promise { - this.treeDataProvider.updateTreeItemContextValue(element); + async refreshTreeView(completedQuery: CompletedQuery): Promise { + this.treeDataProvider.refresh(completedQuery); } } diff --git a/extensions/ql-vscode/src/query-results.ts b/extensions/ql-vscode/src/query-results.ts index 9a4fae16f11..3203e27ec64 100644 --- a/extensions/ql-vscode/src/query-results.ts +++ b/extensions/ql-vscode/src/query-results.ts @@ -1,4 +1,4 @@ -import { env, TreeItem } from 'vscode'; +import { env } from 'vscode'; import { QueryWithResults, tmpDir, QueryInfo } from './run-queries'; import * as messages from './pure/messages'; @@ -17,7 +17,6 @@ export class CompletedQuery implements QueryWithResults { readonly database: DatabaseInfo; readonly logFileLocation?: string; options: QueryHistoryItemOptions; - treeItem?: TreeItem; dispose: () => void; /** From 081d5bcb12042579dcd3f4aa4fe4d4894e6d6750 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 15 Dec 2020 16:23:28 -0800 Subject: [PATCH 2/2] Add unit tests for query-history --- extensions/ql-vscode/src/query-history.ts | 8 +-- .../no-workspace/query-history.test.ts | 66 ++++++++++++++++++- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/query-history.ts b/extensions/ql-vscode/src/query-history.ts index fbdbe82c934..ffe044a354e 100644 --- a/extensions/ql-vscode/src/query-history.ts +++ b/extensions/ql-vscode/src/query-history.ts @@ -55,7 +55,7 @@ const FAILED_QUERY_HISTORY_ITEM_ICON = 'media/red-x.svg'; /** * Tree data provider for the query history view. */ -class HistoryTreeDataProvider extends DisposableObject { +export class HistoryTreeDataProvider extends DisposableObject { private _onDidChangeTreeData = super.push(new vscode.EventEmitter()); readonly onDidChangeTreeData: vscode.Event = this @@ -144,8 +144,8 @@ class HistoryTreeDataProvider extends DisposableObject { return this.history; } - refresh(CompletedQuery?: CompletedQuery) { - this._onDidChangeTreeData.fire(CompletedQuery); + refresh(completedQuery?: CompletedQuery) { + this._onDidChangeTreeData.fire(completedQuery); } find(queryId: number): CompletedQuery | undefined { @@ -356,7 +356,7 @@ export class QueryHistoryManager extends DisposableObject { if (response !== undefined) { // Interpret empty string response as 'go back to using default' singleItem.options.label = response === '' ? undefined : response; - this.treeDataProvider.refresh(); + this.treeDataProvider.refresh(singleItem); } } diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts index 01586407f33..b5218ff7522 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts @@ -5,7 +5,9 @@ import * as vscode from 'vscode'; import * as sinon from 'sinon'; import * as chaiAsPromised from 'chai-as-promised'; import { logger } from '../../logging'; -import { QueryHistoryManager } from '../../query-history'; +import { QueryHistoryManager, HistoryTreeDataProvider } from '../../query-history'; +import { CompletedQuery } from '../../query-results'; +import { QueryInfo } from '../../run-queries'; chai.use(chaiAsPromised); const expect = chai.expect; @@ -43,7 +45,6 @@ describe('query-history', () => { }); describe('tryOpenExternalFile', () => { - it('should open an external file', async () => { await tryOpenExternalFile('xxx'); expect(showTextDocumentSpy).to.have.been.calledOnceWith( @@ -204,6 +205,67 @@ describe('query-history', () => { expect(queryHistory.compareWithItem).to.be.eq('a'); }); }); + + describe('HistoryTreeDataProvider', () => { + let historyTreeDataProvider: HistoryTreeDataProvider; + beforeEach(() => { + historyTreeDataProvider = new HistoryTreeDataProvider(vscode.Uri.file('/a/b/c').fsPath); + }); + + it('should get a tree item with raw results', async () => { + const mockQuery = { + query: { + hasInterpretedResults: () => Promise.resolve(false) + } as QueryInfo, + didRunSuccessfully: true, + toString: () => 'mock label' + } as CompletedQuery; + const treeItem = await historyTreeDataProvider.getTreeItem(mockQuery); + expect(treeItem.command).to.deep.eq({ + title: 'Query History Item', + command: 'codeQLQueryHistory.itemClicked', + arguments: [mockQuery], + }); + expect(treeItem.label).to.eq('mock label'); + expect(treeItem.contextValue).to.eq('rawResultsItem'); + expect(treeItem.iconPath).to.be.undefined; + }); + + it('should get a tree item with interpreted results', async () => { + const mockQuery = { + query: { + // as above, except for this line + hasInterpretedResults: () => Promise.resolve(true) + } as QueryInfo, + didRunSuccessfully: true, + toString: () => 'mock label' + } as CompletedQuery; + const treeItem = await historyTreeDataProvider.getTreeItem(mockQuery); + expect(treeItem.contextValue).to.eq('interpretedResultsItem'); + }); + + it('should get a tree item that did not complete successfully', async () => { + const mockQuery = { + query: { + hasInterpretedResults: () => Promise.resolve(true) + } as QueryInfo, + // as above, except for this line + didRunSuccessfully: false, + toString: () => 'mock label' + } as CompletedQuery; + const treeItem = await historyTreeDataProvider.getTreeItem(mockQuery); + expect(treeItem.iconPath).to.eq(vscode.Uri.file('/a/b/c/media/red-x.svg').fsPath); + }); + + it('should get children', () => { + const mockQuery = { + databaseName: 'abc' + } as CompletedQuery; + historyTreeDataProvider.allHistory.push(mockQuery); + expect(historyTreeDataProvider.getChildren()).to.deep.eq([mockQuery]); + expect(historyTreeDataProvider.getChildren(mockQuery)).to.deep.eq([]); + }); + }); }); function createMockQueryHistory(allHistory: {}[]) {