Skip to content

Simplify selected results table handling #169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Simplify selected results table handling #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions extensions/ql-vscode/src/view/alert-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import cx from 'classnames';
import * as path from 'path';
import * as React from 'react';
import * as Sarif from 'sarif';
import { LocationStyle, ResolvableLocationValue } from 'semmle-bqrs';
import * as octicons from './octicons';
import { className, renderLocation, ResultTableProps, selectedClassName, zebraStripe } from './result-table-utils';
import { className, renderLocation, ResultTableProps, zebraStripe } from './result-table-utils';
import { PathTableResultSet } from './results';

export type PathTableProps = ResultTableProps & { resultSet: PathTableResultSet };
Expand Down Expand Up @@ -99,11 +98,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
}

render(): JSX.Element {
const { selected, databaseUri, resultSet } = this.props;

const tableClassName = cx(className, {
[selectedClassName]: selected
});
const { databaseUri, resultSet } = this.props;

const rows: JSX.Element[] = [];
const { numTruncatedResults, sourceLocationPrefix } = resultSet;
Expand Down Expand Up @@ -342,7 +337,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
</td></tr>);
}

return <table className={tableClassName}>
return <table className={className}>
<tbody>{rows}</tbody>
</table>;
}
Expand Down
11 changes: 3 additions & 8 deletions extensions/ql-vscode/src/view/raw-results-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import cx from 'classnames';
import * as React from "react";
import { className, renderLocation, ResultTableProps, selectedClassName, zebraStripe } from "./result-table-utils";
import { renderLocation, ResultTableProps, zebraStripe, className } from "./result-table-utils";
import { RawTableResultSet, ResultValue, vscode } from "./results";
import { assertNever } from "../helpers-pure";
import { SortDirection, SortState, RAW_RESULTS_LIMIT } from "../interface-types";
Expand All @@ -16,11 +15,7 @@ export class RawTable extends React.Component<RawTableProps, {}> {
}

render(): React.ReactNode {
const { resultSet, selected, databaseUri } = this.props;

const tableClassName = cx(className, {
[selectedClassName]: selected
});
const { resultSet, databaseUri } = this.props;

let dataRows = this.props.resultSet.rows;
let numTruncatedResults = 0;
Expand Down Expand Up @@ -52,7 +47,7 @@ export class RawTable extends React.Component<RawTableProps, {}> {
</td></tr>);
}

return <table className={tableClassName}>
return <table className={className}>
<thead>
<tr>
{
Expand Down
3 changes: 0 additions & 3 deletions extensions/ql-vscode/src/view/result-table-utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { SortState } from '../interface-types';
import { ResultSet, vscode } from './results';

export interface ResultTableProps {
selected: boolean;
resultSet: ResultSet;
databaseUri: string;
resultsPath: string | undefined;
Expand All @@ -14,8 +13,6 @@ export interface ResultTableProps {
export const className = 'vscode-codeql__result-table';
export const tableSelectionHeaderClassName = 'vscode-codeql__table-selection-header';
export const toggleDiagnosticsClassName = `${className}-toggle-diagnostics`;
export const selectedClassName = `${className}--selected`;
export const toggleDiagnosticsSelectedClassName = `${toggleDiagnosticsClassName}--selected`;
export const evenRowClassName = 'vscode-codeql__result-table-row--even';
export const oddRowClassName = 'vscode-codeql__result-table-row--odd';
export const pathRowClassName = 'vscode-codeql__result-table-row--path';
Expand Down
51 changes: 26 additions & 25 deletions extensions/ql-vscode/src/view/result-tables.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import cx from 'classnames';
import * as React from 'react';
import { DatabaseInfo, Interpretation, SortState } from '../interface-types';
import { PathTable } from './alert-table';
import { RawTable } from './raw-results-table';
import { ResultTableProps, toggleDiagnosticsClassName, toggleDiagnosticsSelectedClassName, tableSelectionHeaderClassName } from './result-table-utils';
import { ResultTableProps, tableSelectionHeaderClassName, toggleDiagnosticsClassName } from './result-table-utils';
import { ResultSet, vscode } from './results';

/**
Expand Down Expand Up @@ -76,14 +75,26 @@ export class ResultTables
}

render(): React.ReactNode {
const selectedTable = this.state.selectedTable;
const { selectedTable } = this.state;
const resultSets = this.getResultSets();
const { database, resultsPath, kind } = this.props;

// Only show the Problems view display checkbox for the alerts table.
const toggleDiagnosticsClass = cx(toggleDiagnosticsClassName, {
[toggleDiagnosticsSelectedClassName]: selectedTable === ALERTS_TABLE_NAME
});
const diagnosticsCheckBox = selectedTable === ALERTS_TABLE_NAME ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use if instead of ternary so it's easier to read, since the assigned value here has a lot of text!

Copy link
Contributor Author

@jcreedcmu jcreedcmu Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care for the fact that

  let val: SomeType | undefined = undefined;
  if (condition)
    val = someComputation();

requires a type annotation, and makes val mutable, when I know that for the rest of the function it should be const. Would you be ok with the idiom condition && someComputation() instead of the ternary, since at least in that case there isn't an undefined stranted after the someComputation()? (I didn't realize until I looked it up just now that react also renders false as nothing, just as it does undefined)

(A utility function renderIf<T>(condition: boolean, value: T): T | undefined could work, though it would have the undesirable consequence that value would be computed regardless of the condition, and

renderIf<T>(condition: boolean, computation: () => T): T | undefined {
  if (condition) return value(); else return undefined;
}

would of course solve that, but that would make you have to throw extra () => in every callsite)

Copy link
Contributor Author

@jcreedcmu jcreedcmu Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@henrymercer agreed in an earlier conversation that it's unfortunate typescript lacks good concrete syntax for conditionals and switch statements as expressions :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. The minor increase in readability is not worth any of these :) I do appreciate the thorough investigation of options.

<div className={toggleDiagnosticsClassName}>
<input type="checkbox" id="toggle-diagnostics" name="toggle-diagnostics" onChange={(e) => {
if (resultsPath !== undefined) {
vscode.postMessage({
t: 'toggleDiagnostics',
resultsPath: resultsPath,
databaseUri: database.databaseUri,
visible: e.target.checked,
kind: kind
});
}
}} />
<label htmlFor="toggle-diagnostics">Show results in Problems view</label>
</div> : undefined;

return <div>
<div className={tableSelectionHeaderClassName}>
Expand All @@ -96,20 +107,7 @@ export class ResultTables
)
}
</select>
<div className={toggleDiagnosticsClass}>
<input type="checkbox" id="toggle-diagnostics" name="toggle-diagnostics" onChange={(e) => {
if (resultsPath !== undefined) {
vscode.postMessage({
t: 'toggleDiagnostics',
resultsPath: resultsPath,
databaseUri: database.databaseUri,
visible: e.target.checked,
kind: kind
});
}
}} />
<label htmlFor="toggle-diagnostics">Show results in Problems view</label>
</div>
{diagnosticsCheckBox}
{
this.props.isLoadingNewResults ?
<span className={UPDATING_RESULTS_TEXT_CLASS_NAME}>Updating results…</span>
Expand All @@ -118,9 +116,12 @@ export class ResultTables
</div>
{
resultSets.map(resultSet =>
<ResultTable key={resultSet.schema.name} resultSet={resultSet}
databaseUri={this.props.database.databaseUri} selected={resultSet.schema.name === selectedTable}
resultsPath={this.props.resultsPath} sortState={this.props.sortStates.get(resultSet.schema.name)} />
resultSet.schema.name === selectedTable ?
<ResultTable key={resultSet.schema.name} resultSet={resultSet}
databaseUri={this.props.database.databaseUri}
resultsPath={this.props.resultsPath}
sortState={this.props.sortStates.get(resultSet.schema.name)} /> :
undefined
)
}
</div>;
Expand All @@ -137,10 +138,10 @@ class ResultTable extends React.Component<ResultTableProps, {}> {
const { resultSet } = this.props;
switch (resultSet.t) {
case 'RawResultSet': return <RawTable
selected={this.props.selected} resultSet={resultSet} databaseUri={this.props.databaseUri}
resultSet={resultSet} databaseUri={this.props.databaseUri}
resultsPath={this.props.resultsPath} sortState={this.props.sortState} />;
case 'SarifResultSet': return <PathTable
selected={this.props.selected} resultSet={resultSet} databaseUri={this.props.databaseUri}
resultSet={resultSet} databaseUri={this.props.databaseUri}
resultsPath={this.props.resultsPath} />;
}
}
Expand Down
18 changes: 5 additions & 13 deletions extensions/ql-vscode/src/view/resultsView.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
.vscode-codeql__result-table {
display: none;
display: table;
border-collapse: collapse;
width: 100%;
}

.vscode-codeql__result-table--selected {
display: table;
}

.vscode-codeql__table-selection-header {
display: flex;
padding: 0.5em 0;
Expand All @@ -18,22 +14,18 @@
}

.vscode-codeql__result-table-toggle-diagnostics {
display: none;
display: inline-block;
text-align: left;
margin-left: auto;
}

.vscode-codeql__result-table-toggle-diagnostics--selected {
display: inline-block;
}

/* Keep the checkbox and its label in horizontal alignment. */
.vscode-codeql__result-table-toggle-diagnostics--selected label,
.vscode-codeql__result-table-toggle-diagnostics--selected input {
.vscode-codeql__result-table-toggle-diagnostics label,
.vscode-codeql__result-table-toggle-diagnostics input {
display: inline-block;
vertical-align: middle;
}
.vscode-codeql__result-table-toggle-diagnostics--selected input {
.vscode-codeql__result-table-toggle-diagnostics input {
margin: 3px 3px 1px 3px;
}

Expand Down