Fix sort order and selection

This commit fixes two related issues with the
history view.

1. Sort order was changing after a query item completed. The fix is a
   change in how we fire off the `onDidChangeTreeData` event. When the
   event is fired with a single item, that item is pushed to the top of
   the list. I'm not exactly sure why this wasn't happening before, but
   I suspect it was because we were refreshing the list at the same time
   as we were inserting the new item.

   The solution here is to always refresh the entire list, instead of
   single items. This is fine since re building the list is a trivial
   operation. See the `refreshTreeView()` method.

   With this change, the sort order is now stable.

2. Originally reported here: #1093
   The problem is that the internal treeView selection was not being
   updated when a new item was being added. Due to some oddities with
   the way selection works in the tree view (ie- the visible selection
   does not always match the internal selection).

   The solution is to use the current item from the `treeDataProvider`
   in `determineSelection`.

Also, this change makes the sorting more precise and fixes some typos.

Author: aeisenberg

extensions/ql-vscode/src/compare/compare-interface.ts

@@ -95,7 +95,7 @@ export class CompareInterfaceManager extends DisposableObject {
         currentResultSetName: currentResultSetName,
         rows,
         message,
-        datebaseUri: to.initialInfo.databaseInfo.databaseUri,
+        databaseUri: to.initialInfo.databaseInfo.databaseUri,
       });
     }
   }

extensions/ql-vscode/src/compare/view/Compare.tsx

@@ -17,7 +17,7 @@ const emptyComparison: SetComparisonsMessage = {
   columns: [],
   commonResultSetNames: [],
   currentResultSetName: '',
-  datebaseUri: '',
+  databaseUri: '',
   message: 'Empty comparison'
 };
 

extensions/ql-vscode/src/compare/view/CompareTable.tsx

@@ -76,7 +76,7 @@ export default function CompareTable(props: Props) {
                 schemaName={comparison.currentResultSetName}
                 preventSort={true}
               />
-              {createRows(rows.from, comparison.datebaseUri)}
+              {createRows(rows.from, comparison.databaseUri)}
             </table>
           </td>
           <td>
@@ -86,7 +86,7 @@ export default function CompareTable(props: Props) {
                 schemaName={comparison.currentResultSetName}
                 preventSort={true}
               />
-              {createRows(rows.to, comparison.datebaseUri)}
+              {createRows(rows.to, comparison.databaseUri)}
             </table>
           </td>
         </tr>

extensions/ql-vscode/src/extension.ts

@@ -499,8 +499,6 @@ async function activateWithInstalledDistribution(
       const initialInfo = await createInitialQueryInfo(selectedQuery, databaseInfo, quickEval, range);
       const item = new FullQueryInfo(initialInfo, queryHistoryConfigurationListener);
       qhm.addQuery(item);
-      await qhm.refreshTreeView(item);
-
       try {
         const completedQueryInfo = await compileAndRunQueryAgainstDatabase(
           cliServer,
@@ -518,7 +516,7 @@ async function activateWithInstalledDistribution(
         item.failureReason = e.message;
         throw e;
       } finally {
-        await qhm.refreshTreeView(item);
+        qhm.refreshTreeView();
       }
     }
   }

extensions/ql-vscode/src/interface.ts

@@ -33,7 +33,7 @@ import { Logger } from './logging';
 import * as messages from './pure/messages';
 import { commandRunner } from './commandRunner';
 import { CompletedQueryInfo, interpretResults } from './query-results';
-import { QueryEvaluatonInfo, tmpDir } from './run-queries';
+import { QueryEvaluationInfo, tmpDir } from './run-queries';
 import { parseSarifLocation, parseSarifPlainTextMessage } from './pure/sarif-utils';
 import {
   WebviewReveal,
@@ -644,7 +644,7 @@ export class InterfaceManager extends DisposableObject {
   }
 
   private async interpretResultsInfo(
-    query: QueryEvaluatonInfo,
+    query: QueryEvaluationInfo,
     sortState: InterpretedResultsSortState | undefined
   ): Promise<Interpretation | undefined> {
     if (

extensions/ql-vscode/src/pure/interface-types.ts

@@ -316,7 +316,7 @@ export interface SetComparisonsMessage {
   readonly currentResultSetName: string;
   readonly rows: QueryCompareResult | undefined;
   readonly message: string | undefined;
-  readonly datebaseUri: string;
+  readonly databaseUri: string;
 }
 
 export enum DiffKind {

extensions/ql-vscode/src/query-history.ts

@@ -96,9 +96,6 @@ export class HistoryTreeDataProvider extends DisposableObject {
 
   private localSuccessIconPath: string;
 
-  /**
-   * When not undefined, must be reference-equal to an item in `this.databases`.
-   */
   private current: FullQueryInfo | undefined;
 
   constructor(extensionPath: string) {
@@ -152,8 +149,8 @@ export class HistoryTreeDataProvider extends DisposableObject {
     element?: FullQueryInfo
   ): ProviderResult<FullQueryInfo[]> {
     return element ? [] : this.history.sort((h1, h2) => {
-      const q1 = h1.completedQuery;
-      const q2 = h2.completedQuery;
+      const resultCount1 = h1.completedQuery?.resultCount ?? -1;
+      const resultCount2 = h2.completedQuery?.resultCount ?? -1;
 
       switch (this.sortOrder) {
         case SortOrder.NameAsc:
@@ -165,9 +162,15 @@ export class HistoryTreeDataProvider extends DisposableObject {
         case SortOrder.DateDesc:
           return h2.initialInfo.start.getTime() - h1.initialInfo.start.getTime();
         case SortOrder.CountAsc:
-          return (!q1 || !q2) ? 0 : q1.resultCount - q2.resultCount;
+          // If the result counts are equal, sort by name.
+          return resultCount1 - resultCount2 === 0
+            ? h1.label.localeCompare(h2.label, env.language)
+            : resultCount1 - resultCount2;
         case SortOrder.CountDesc:
-          return (!q1 || !q2) ? 0 : q2.resultCount - q1.resultCount;
+          // If the result counts are equal, sort by name.
+          return resultCount2 - resultCount1 === 0
+            ? h2.label.localeCompare(h1.label, env.language)
+            : resultCount2 - resultCount1;
         default:
           assertNever(this.sortOrder);
       }
@@ -183,8 +186,8 @@ export class HistoryTreeDataProvider extends DisposableObject {
   }
 
   pushQuery(item: FullQueryInfo): void {
-    this.current = item;
     this.history.push(item);
+    this.setCurrentItem(item);
     this.refresh();
   }
 
@@ -193,16 +196,17 @@ export class HistoryTreeDataProvider extends DisposableObject {
   }
 
   remove(item: FullQueryInfo) {
-    if (this.current === item) {
-      this.current = undefined;
+    const isCurrent = this.current === item;
+    if (isCurrent) {
+      this.setCurrentItem();
     }
     const index = this.history.findIndex((i) => i === item);
     if (index >= 0) {
       this.history.splice(index, 1);
-      if (this.current === undefined && this.history.length > 0) {
+      if (isCurrent && this.history.length > 0) {
         // Try to keep a current item, near the deleted item if there
         // are any available.
-        this.current = this.history[Math.min(index, this.history.length - 1)];
+        this.setCurrentItem(this.history[Math.min(index, this.history.length - 1)]);
       }
       this.refresh();
     }
@@ -212,8 +216,8 @@ export class HistoryTreeDataProvider extends DisposableObject {
     return this.history;
   }
 
-  refresh(item?: FullQueryInfo) {
-    this._onDidChangeTreeData.fire(item);
+  refresh() {
+    this._onDidChangeTreeData.fire(undefined);
   }
 
   public get sortOrder() {
@@ -267,11 +271,13 @@ export class QueryHistoryManager extends DisposableObject {
         this.updateTreeViewSelectionIfVisible()
       )
     );
-    // Don't allow the selection to become empty
     this.push(
       this.treeView.onDidChangeSelection(async (ev) => {
-        if (ev.selection.length == 0) {
+        if (ev.selection.length === 0) {
+          // Don't allow the selection to become empty
           this.updateTreeViewSelectionIfVisible();
+        } else {
+          this.treeDataProvider.setCurrentItem(ev.selection[0]);
         }
         this.updateCompareWith(ev.selection);
       })
@@ -433,12 +439,15 @@ export class QueryHistoryManager extends DisposableObject {
     const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect);
 
     (finalMultiSelect || [finalSingleItem]).forEach((item) => {
-      this.treeDataProvider.remove(item);
-      item.completedQuery?.dispose();
+      // TODO: Removing in progress queries is not supported yet
+      if (item.status !== QueryStatus.InProgress) {
+        this.treeDataProvider.remove(item);
+        item.completedQuery?.dispose();
+      }
     });
     const current = this.treeDataProvider.getCurrent();
     if (current !== undefined) {
-      await this.treeView.reveal(current);
+      await this.treeView.reveal(current, { select: true });
       await this.invokeCallbackOn(current);
     }
   }
@@ -484,12 +493,7 @@ export class QueryHistoryManager extends DisposableObject {
     if (response !== undefined) {
       // Interpret empty string response as 'go back to using default'
       singleItem.initialInfo.userSpecifiedLabel = response === '' ? undefined : response;
-      if (this.treeDataProvider.sortOrder === SortOrder.NameAsc ||
-        this.treeDataProvider.sortOrder === SortOrder.NameDesc) {
-        this.treeDataProvider.refresh();
-      } else {
-        this.treeDataProvider.refresh(singleItem);
-      }
+      this.treeDataProvider.refresh();
     }
   }
 
@@ -685,7 +689,7 @@ export class QueryHistoryManager extends DisposableObject {
         // We must fire the onDidChangeTreeData event to ensure the current element can be selected
         // using `reveal` if the tree view was not visible when the current element was added.
         this.treeDataProvider.refresh();
-        void this.treeView.reveal(current);
+        void this.treeView.reveal(current, { select: true });
       }
     }
   }
@@ -826,13 +830,19 @@ the file in the file explorer and dragging it into the workspace.`
     singleItem: FullQueryInfo,
     multiSelect: FullQueryInfo[]
   ): { finalSingleItem: FullQueryInfo; finalMultiSelect: FullQueryInfo[] } {
-    if (singleItem === undefined && (multiSelect === undefined || multiSelect.length === 0 || multiSelect[0] === undefined)) {
+    if (!singleItem && !multiSelect?.[0]) {
       const selection = this.treeView.selection;
-      if (selection) {
+      const current = this.treeDataProvider.getCurrent();
+      if (selection?.length) {
         return {
           finalSingleItem: selection[0],
           finalMultiSelect: selection
         };
+      } else if (current) {
+        return {
+          finalSingleItem: current,
+          finalMultiSelect: [current]
+        };
       }
     }
     return {
@@ -841,7 +851,7 @@ the file in the file explorer and dragging it into the workspace.`
     };
   }
 
-  async refreshTreeView(item: FullQueryInfo): Promise<void> {
-    this.treeDataProvider.refresh(item);
+  refreshTreeView(): void {
+    this.treeDataProvider.refresh();
   }
 }

extensions/ql-vscode/src/query-results.ts

@@ -1,6 +1,6 @@
 import { env } from 'vscode';
 
-import { QueryWithResults, tmpDir, QueryEvaluatonInfo } from './run-queries';
+import { QueryWithResults, tmpDir, QueryEvaluationInfo } from './run-queries';
 import * as messages from './pure/messages';
 import * as cli from './cli';
 import * as sarif from 'sarif';
@@ -39,7 +39,7 @@ export enum QueryStatus {
 }
 
 export class CompletedQueryInfo implements QueryWithResults {
-  readonly query: QueryEvaluatonInfo;
+  readonly query: QueryEvaluationInfo;
   readonly result: messages.EvaluationResult;
   readonly logFileLocation?: string;
   resultCount: number;

extensions/ql-vscode/src/run-queries.ts

@@ -53,7 +53,7 @@ export const tmpDirDisposal = {
  * temporary files associated with it, such as the compiled query
  * output and results.
  */
-export class QueryEvaluatonInfo {
+export class QueryEvaluationInfo {
   readonly compiledQueryPath: string;
   readonly dilPath: string;
   readonly csvPath: string;
@@ -266,7 +266,7 @@ export class QueryEvaluatonInfo {
 
 
 export interface QueryWithResults {
-  readonly query: QueryEvaluatonInfo;
+  readonly query: QueryEvaluationInfo;
   readonly result: messages.EvaluationResult;
   readonly logFileLocation?: string;
   readonly dispose: () => void;
@@ -351,7 +351,7 @@ async function getSelectedPosition(editor: TextEditor, range?: Range): Promise<m
 async function checkDbschemeCompatibility(
   cliServer: cli.CodeQLCliServer,
   qs: qsClient.QueryServerClient,
-  query: QueryEvaluatonInfo,
+  query: QueryEvaluationInfo,
   progress: ProgressCallback,
   token: CancellationToken,
 ): Promise<void> {
@@ -393,7 +393,7 @@ async function checkDbschemeCompatibility(
   }
 }
 
-function reportNoUpgradePath(query: QueryEvaluatonInfo) {
+function reportNoUpgradePath(query: QueryEvaluationInfo) {
   throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace.\n\nPlease try using a newer version of the query libraries.`);
 }
 
@@ -403,7 +403,7 @@ function reportNoUpgradePath(query: QueryEvaluatonInfo) {
 async function compileNonDestructiveUpgrade(
   qs: qsClient.QueryServerClient,
   upgradeTemp: tmp.DirectoryResult,
-  query: QueryEvaluatonInfo,
+  query: QueryEvaluationInfo,
   progress: ProgressCallback,
   token: CancellationToken,
 ): Promise<string> {
@@ -614,7 +614,7 @@ export async function compileAndRunQueryAgainstDatabase(
     }
   }
 
-  const query = new QueryEvaluatonInfo(initialInfo.id, qlProgram, db, packConfig.dbscheme, initialInfo.quickEvalPosition, metadata, templates);
+  const query = new QueryEvaluationInfo(initialInfo.id, qlProgram, db, packConfig.dbscheme, initialInfo.quickEvalPosition, metadata, templates);
 
   const upgradeDir = await tmp.dir({ dir: upgradesTmpDir.name, unsafeCleanup: true });
   try {
@@ -716,7 +716,7 @@ const compilationFailedErrorTail = ' compilation failed. Please make sure there
   ' and choose CodeQL Query Server from the dropdown.';
 
 function createSyntheticResult(
-  query: QueryEvaluatonInfo,
+  query: QueryEvaluationInfo,
   message: string,
   resultType: number
 ): QueryWithResults {

extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts

@@ -6,7 +6,7 @@ import * as sinon from 'sinon';
 import * as chaiAsPromised from 'chai-as-promised';
 import { logger } from '../../logging';
 import { QueryHistoryManager, HistoryTreeDataProvider } from '../../query-history';
-import { QueryEvaluatonInfo, QueryWithResults } from '../../run-queries';
+import { QueryEvaluationInfo, QueryWithResults } from '../../run-queries';
 import { QueryHistoryConfigListener } from '../../config';
 import * as messages from '../../pure/messages';
 import { QueryServerClient } from '../../queryserver-client';
@@ -379,7 +379,7 @@ describe('query-history', () => {
     return {
       query: {
         hasInterpretedResults: () => Promise.resolve(hasInterpretedResults)
-      } as QueryEvaluatonInfo,
+      } as QueryEvaluationInfo,
       result: {
         resultType: didRunSuccessfully
           ? messages.QueryResultType.SUCCESS