Switch commands to up/down/left/right semantics

Author: asgerf

extensions/ql-vscode/package.json

@@ -595,19 +595,19 @@
       },
       {
         "command": "codeQLQueryResults.nextPathStep",
-        "title": "CodeQL: Show Next Step on Path"
+        "title": "CodeQL: Navigate down in result viewer"
       },
       {
         "command": "codeQLQueryResults.previousPathStep",
-        "title": "CodeQL: Show Previous Step on Path"
+        "title": "CodeQL: Navigate up in result viewer"
       },
       {
-        "command": "codeQLQueryResults.nextAlert",
-        "title": "CodeQL: Show Next Alert"
+        "command": "codeQLQueryResults.right",
+        "title": "CodeQL: Navigate right in result viewer"
       },
       {
-        "command": "codeQLQueryResults.previousAlert",
-        "title": "CodeQL: Show Previous Alert"
+        "command": "codeQLQueryResults.left",
+        "title": "CodeQL: Navigate left in result viewer"
       },
       {
         "command": "codeQL.restartQueryServer",

extensions/ql-vscode/src/interface.ts

@@ -27,6 +27,7 @@ import {
   ALERTS_TABLE_NAME,
   GRAPH_TABLE_NAME,
   RawResultsSortState,
+  NavigationDirection,
 } from './pure/interface-types';
 import { Logger } from './logging';
 import { commandRunner } from './commandRunner';
@@ -144,26 +145,26 @@ export class ResultsView extends AbstractWebview<IntoResultsViewMsg, FromResults
     void logger.log('Registering path-step navigation commands.');
     this.push(
       commandRunner(
-        'codeQLQueryResults.nextPathStep',
-        this.navigatePathStep.bind(this, 1)
+        'codeQLQueryResults.nextPathStep', // TODO: deprecate the old commands and make them forward to new commands named 'up/down'
+        this.navigateResultView.bind(this, NavigationDirection.down)
       )
     );
     this.push(
       commandRunner(
         'codeQLQueryResults.previousPathStep',
-        this.navigatePathStep.bind(this, -1)
+        this.navigateResultView.bind(this, NavigationDirection.up)
       )
     );
     this.push(
       commandRunner(
-        'codeQLQueryResults.nextAlert',
-        this.navigateAlert.bind(this, 1)
+        'codeQLQueryResults.right',
+        this.navigateResultView.bind(this, NavigationDirection.right)
       )
     );
     this.push(
       commandRunner(
-        'codeQLQueryResults.previousAlert',
-        this.navigateAlert.bind(this, -1)
+        'codeQLQueryResults.left',
+        this.navigateResultView.bind(this, NavigationDirection.left)
       )
     );
 
@@ -181,12 +182,8 @@ export class ResultsView extends AbstractWebview<IntoResultsViewMsg, FromResults
     );
   }
 
-  async navigatePathStep(direction: number): Promise<void> {
-    await this.postMessage({ t: 'navigatePath', direction });
-  }
-
-  async navigateAlert(direction: number): Promise<void> {
-    await this.postMessage({ t: 'navigateAlert', direction });
+  async navigateResultView(direction: NavigationDirection): Promise<void> {
+    await this.postMessage({ t: 'navigate', direction });
   }
 
   protected getPanelConfig(): WebviewPanelConfig {

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

@@ -141,20 +141,17 @@ export interface ShowInterpretedPageMsg {
   queryPath: string;
 }
 
-/** Advance to the next or previous path step in the path viewer */
-export interface NavigatePathMsg {
-  t: 'navigatePath';
-
-  /** 1 for next, -1 for previous */
-  direction: number;
+export const enum NavigationDirection {
+  up = 'up',
+  down = 'down',
+  left = 'left',
+  right = 'right',
 }
 
-/** Advance to the next or previous alert in the path viewer */
-export interface NavigateAlertMsg {
-  t: 'navigateAlert';
-
-  /** 1 for next, -1 for previous */
-  direction: number;
+/** Move up, down, left, or right in the result viewer. */
+export interface NavigateMsg {
+  t: 'navigate';
+  direction: NavigationDirection;
 }
 
 /**
@@ -172,8 +169,7 @@ export type IntoResultsViewMsg =
   | ResultsUpdatingMsg
   | SetStateMsg
   | ShowInterpretedPageMsg
-  | NavigatePathMsg
-  | NavigateAlertMsg
+  | NavigateMsg
   | UntoggleShowProblemsMsg;
 
 /**

extensions/ql-vscode/src/view/results/RawTableRow.tsx

@@ -1,24 +1,24 @@
 import * as React from 'react';
 import { ResultRow } from '../../pure/bqrs-cli-types';
-import { selectableZebraStripe } from './result-table-utils';
+import { selectedRowClassName, zebraStripe } from './result-table-utils';
 import RawTableValue from './RawTableValue';
 
 interface Props {
   rowIndex: number;
   row: ResultRow;
   databaseUri: string;
   className?: string;
-  isSelected?: boolean;
+  selectedColumn?: number;
   onSelected?: (row: number, column: number) => void;
 }
 
 export default function RawTableRow(props: Props) {
   return (
-    <tr key={props.rowIndex} {...selectableZebraStripe(props.isSelected ?? false, props.rowIndex, props.className || '')}>
+    <tr key={props.rowIndex} {...zebraStripe(props.rowIndex, props.className || '')}>
       <td key={-1}>{props.rowIndex + 1}</td>
 
       {props.row.map((value, columnIndex) => (
-        <td key={columnIndex}>
+        <td key={columnIndex} {...props.selectedColumn === columnIndex ? { className: selectedRowClassName } : {}}>
           <RawTableValue
             value={value}
             databaseUri={props.databaseUri}

extensions/ql-vscode/src/view/results/alert-table.tsx

@@ -3,9 +3,9 @@ import * as React from 'react';
 import * as Sarif from 'sarif';
 import * as Keys from '../../pure/result-keys';
 import * as octicons from './octicons';
-import { className, renderLocation, ResultTableProps, zebraStripe, selectableZebraStripe, jumpToLocation, nextSortDirection, emptyQueryResultsMessage } from './result-table-utils';
-import { onNavigation, NavigationEvent } from './results';
-import { InterpretedResultSet, NavigateAlertMsg, NavigatePathMsg, SarifInterpretationData } from '../../pure/interface-types';
+import { className, renderLocation, ResultTableProps, selectableZebraStripe, jumpToLocation, nextSortDirection, emptyQueryResultsMessage } from './result-table-utils';
+import { onNavigation } from './results';
+import { InterpretedResultSet, NavigateMsg, NavigationDirection, SarifInterpretationData } from '../../pure/interface-types';
 import {
   parseSarifPlainTextMessage,
   parseSarifLocation,
@@ -18,7 +18,7 @@ import { isWholeFileLoc, isLineColumnLoc } from '../../pure/bqrs-utils';
 export type PathTableProps = ResultTableProps & { resultSet: InterpretedResultSet<SarifInterpretationData> };
 export interface PathTableState {
   expanded: Set<string>;
-  selectedItem: undefined | Keys.PathNode | Keys.Result;
+  selectedItem: undefined | Keys.ResultKey;
 }
 
 export class PathTable extends React.Component<PathTableProps, PathTableState> {
@@ -246,8 +246,9 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
           const currentPathExpanded = this.state.expanded.has(Keys.keyToString(pathKey));
           if (currentResultExpanded) {
             const indicator = currentPathExpanded ? octicons.chevronDown : octicons.chevronRight;
+            const isPathSpecificallySelected = Keys.equalsNotUndefined(pathKey, selectedItem);
             rows.push(
-              <tr {...zebraStripe(resultIndex)} key={`${resultIndex}-${pathIndex}`}>
+              <tr {...selectableZebraStripe(isPathSpecificallySelected, resultIndex)} key={`${resultIndex}-${pathIndex}`}>
                 <td className="vscode-codeql__icon-cell"><span className="vscode-codeql__vertical-rule"></span></td>
                 <td className="vscode-codeql__icon-cell vscode-codeql__dropdown-cell" onMouseDown={toggler([pathKey])}>{indicator}</td>
                 <td className="vscode-codeql__text-center" colSpan={3}>
@@ -302,82 +303,89 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
     </table>;
   }
 
-  private handleNavigationEvent(event: NavigationEvent) {
-    switch (event.t) {
-      case 'navigatePath': {
-        this.handleNavigatePathStepEvent(event);
-        break;
-      }
-      case 'navigateAlert': {
-        this.handleNavigateAlertEvent(event);
-        break;
-      }
-    }
-  }
-
-  private handleNavigatePathStepEvent(event: NavigatePathMsg) {
+  private handleNavigationEvent(event: NavigateMsg) {
     this.setState(prevState => {
-      const { selectedItem } = prevState;
-      if (selectedItem === undefined) return prevState;
-
-      const selectedPath = selectedItem.pathIndex !== undefined
-        ? selectedItem
-        : { ...selectedItem, pathIndex: 0 };
-
-      const path = Keys.getPath(this.props.resultSet.interpretation.data, selectedPath);
-      if (path === undefined) return prevState;
-
-      const nextIndex = selectedItem.pathNodeIndex !== undefined
-        ? (selectedItem.pathNodeIndex + event.direction)
-        : 0;
-      if (nextIndex < 0 || nextIndex >= path.locations.length) return prevState;
-
-      const sarifLoc = path.locations[nextIndex].location;
-      if (sarifLoc === undefined) {
-        return prevState;
+      const key = this.getNewSelection(prevState.selectedItem, event.direction);
+      const data = this.props.resultSet.interpretation.data;
+
+      // Check if the selected node actually exists (bounds check) and get its location if relevant
+      let jumpLocation: Sarif.Location | undefined;
+      if (key.pathNodeIndex != null) {
+        jumpLocation = Keys.getPathNode(data, key);
+        if (jumpLocation == null) {
+          return prevState; // Result does not exist
+        }
+      } else if (key.pathIndex != null) {
+        if (Keys.getPath(data, key) == null) {
+          return prevState; // Path does not exist
+        }
+        jumpLocation = undefined; // When selecting a 'path', don't jump anywhere.
+      } else {
+        jumpLocation = Keys.getResult(data, key)?.locations?.[0];
+        if (jumpLocation == null) {
+          return prevState; // Path step does not exist.
+        }
       }
-
-      const loc = parseSarifLocation(sarifLoc, this.props.resultSet.interpretation.sourceLocationPrefix);
-      if (isNoLocation(loc)) {
-        return prevState;
+      if (jumpLocation !== undefined) {
+        const parsedLocation = parseSarifLocation(jumpLocation, this.props.resultSet.interpretation.sourceLocationPrefix);
+        if (!isNoLocation(parsedLocation)) {
+          jumpToLocation(parsedLocation, this.props.databaseUri);
+        }
       }
 
-      jumpToLocation(loc, this.props.databaseUri);
-      const newSelection = { ...selectedPath, pathNodeIndex: nextIndex };
-      const newExpanded = new Set(prevState.expanded);
-      // In case we're jumping from the main alert row to its first path step, expand the enclosing nodes so the selected
-      // node is actually visible.
-      newExpanded.add(Keys.keyToString({ resultIndex: newSelection.resultIndex }));
-      newExpanded.add(Keys.keyToString({ resultIndex: newSelection.resultIndex, pathIndex: newSelection.pathIndex }));
-      return { ...prevState, selectedItem: newSelection, expanded: newExpanded };
+      const expanded = new Set(prevState.expanded);
+      if (event.direction === NavigationDirection.right) {
+        // When stepping right, expand to ensure the selected node is visible
+        expanded.add(Keys.keyToString({ resultIndex: key.resultIndex }));
+        if (key.pathIndex != null) {
+          expanded.add(Keys.keyToString({ resultIndex: key.resultIndex, pathIndex: key.pathIndex }));
+        }
+      } else if (event.direction === NavigationDirection.left) {
+        // When stepping left, collapse immediately
+        expanded.delete(Keys.keyToString(key));
+      }
+      return {
+        ...prevState,
+        expanded,
+        selectedItem: key
+      };
     });
   }
 
-  private handleNavigateAlertEvent(event: NavigateAlertMsg) {
-    this.setState(prevState => {
-      const { selectedItem } = prevState;
-      if (selectedItem === undefined) return prevState;
-
-      const nextIndex = selectedItem.resultIndex + event.direction;
-      const nextSelection = { resultIndex: nextIndex };
-      const result = Keys.getResult(this.props.resultSet.interpretation.data, nextSelection);
-      if (result === undefined) {
-        return prevState;
-      }
-
-      const sarifLoc = result.locations?.[0];
-      if (sarifLoc === undefined) {
-        return prevState;
-      }
-
-      const loc = parseSarifLocation(sarifLoc, this.props.resultSet.interpretation.sourceLocationPrefix);
-      if (isNoLocation(loc)) {
-        return prevState;
+  private getNewSelection(key: Keys.ResultKey | undefined, direction: NavigationDirection): Keys.ResultKey {
+    if (key === undefined) {
+      return { resultIndex: 0 };
+    }
+    const { resultIndex, pathIndex, pathNodeIndex } = key;
+    switch (direction) {
+      case NavigationDirection.up:
+      case NavigationDirection.down: {
+        const delta = direction === NavigationDirection.up ? -1 : 1;
+        if (key.pathNodeIndex !== undefined) {
+          return { resultIndex, pathIndex: key.pathIndex, pathNodeIndex: key.pathNodeIndex + delta };
+        } else if (pathIndex !== undefined) {
+          return { resultIndex, pathIndex: pathIndex + delta };
+        } else {
+          return { resultIndex: resultIndex + delta };
+        }
       }
-
-      jumpToLocation(loc, this.props.databaseUri);
-      return { ...prevState, selectedItem: nextSelection };
-    });
+      case NavigationDirection.left:
+        if (key.pathNodeIndex !== undefined) {
+          return { resultIndex, pathIndex: key.pathIndex };
+        } else if (pathIndex !== undefined) {
+          return { resultIndex };
+        } else {
+          return key;
+        }
+      case NavigationDirection.right:
+        if (pathIndex === undefined) {
+          return { resultIndex, pathIndex: 0 };
+        } else if (pathNodeIndex === undefined) {
+          return { resultIndex, pathIndex, pathNodeIndex: 0 };
+        } else {
+          return key;
+        }
+    }
   }
 
   componentDidMount() {