Add compare view #431

aeisenberg · 2020-06-05T22:53:43Z

This PR adds the compare view. See #394.

To compare 2 queries:

Run a query
Run another query that has the same number of columns, but slightly different results
In the query history view, select a query and right click.
Select Compare with...
Choose the second query
The compare view should show up.

Fixes #394

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/product-docs-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

dbartol · 2020-06-06T03:38:45Z

To compare text files, you right click on one, select “Select for Compare”, then right click on another and select “Compare with Selected”. We should probably use the same approach to invoke our diffs.

aeisenberg · 2020-06-09T15:20:19Z

@dbartol I never even knew that feature existed until you mentioned it here. I could try to do something similar. I'd like to first get the basic feature in and then work on ways to improve and fill it out completely.

The way I generally do compare is to select two files, right-click -> compare.

dbartol · 2020-06-10T15:54:16Z

Hah, I didn't realize that multiselect worked for compare.

Apparently, VSCode allows all three ways of doing compare:
Select for compare -> Compare selected
Select two items -> Compare selected
File: Compare Active File With... -> gives you search box with a dropdown for recent files

Related: Apparently multiselect in a TreeView is supported now, so we could do any or all of the three above. I agree that multiselect is the most natural.

aeisenberg · 2020-06-10T16:13:57Z

I've been having a hell of a time getting the second style to work. It's making me think that there may be a problem with vscode (or at the very least my understanding of vscode). I raised this issue: microsoft/vscode#99767

aeisenberg · 2020-06-15T21:33:32Z

The reason why there are so many commits right now is that I rebased on top of #437. When that change gets merged, this one should shrink.

jcreedcmu · 2020-06-16T14:06:25Z

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

@@ -277,10 +301,10 @@ export class QueryHistoryManager {
  constructor(
    ctx: ExtensionContext,
    private queryHistoryConfigListener: QueryHistoryConfig,
-    selectedCallback?: (item: CompletedQuery) => Promise<void>
+    private selectedCallback: (item: CompletedQuery) => Promise<void>,
+    private doCompareCallback: (from: CompletedQuery, to: CompletedQuery) => Promise<void>,
  ) {
    this.ctx = ctx;


any reason not to replace this.ctx = ctx with an argument list field declaration also?

Makes sense. Must have missed that one.

Actually...don't need it to be a class property at all. removed the field.

jcreedcmu

Mostly looks good, I especially like the refactoring-out of result-related components, which would have been a fine idea even apart from needing to reuse them as you do here.

jcreedcmu · 2020-06-16T14:13:27Z

extensions/ql-vscode/src/languageSupport.ts

-    beforeText: /^(\t|[ ])*[ ]\*[^/]*\*\/\s*$/,
-    action: { indentAction: IndentAction.None, removeText: 1 }
-  }
+const onEnterRules: string[] = [


Why are these commented out?

That's a mistake.

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

jcreedcmu · 2020-06-16T14:22:26Z

extensions/ql-vscode/src/view/result-tables.tsx

@@ -274,6 +286,14 @@ class ResultTable extends React.Component<ResultTableProps, {}> {
        {...this.props} resultSet={resultSet} />;
      case 'SarifResultSet': return <PathTable
        {...this.props} resultSet={resultSet} />;
+      default:


This default shouldn't be necessary; the type ResultSet constraints resultSet.t to be either RawResultSet or SarifResultSet, and the lack of default makes typescript's exhaustivity checking a useful signal. Unless perhaps you see some way that we're likely to break type safety and want an informative error message here?

In which case I'd prefer something slightly more verbose like Invalid result set type, but not knowing anything else, I still think the default doesn't belong here at all.

Will remove.

jcreedcmu · 2020-06-16T15:39:57Z

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

+        t: 'setComparisons',
+        stats: {
+          fromQuery: {
+            // since we split the description into several rows


It's not clear to me that the user-defined label case works well here. When I set the label of the query-being-compared to to [%t] custom, what I see is just the query file name and not the string 'custom' appear anywhere in the view.

Hmmm...let me see.

Ugh...silly mistake. Fixed now.

jcreedcmu · 2020-06-16T15:45:07Z

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

+ * @throws Error when:
+ *  1. number of columns do not match
+ *  2. If either query is empty
+ *  3. If the queries are 100% disjoint


When I do a comparison of disjoint queries, the comparison view is forced visible, and I do get an error message saying the results are disjoint, but the comparison view still contains whichever comparison it did before, which I think is slightly misleading. I think I'd prefer --- but would be happy deferring to a follow-up PR if this is at all complicated --- having one of the possible states of the comparison view actually being "results disjoint" as opposed to window.showErrorMessage.

Hmm...right. I should fix that.

There are a few other error states. What I will do is ensure that the compare view will hide the columns and display the error message in this case.

Yeah, that sounds great.

Adds the tsx file and updates the webpack config so that this new tsx file is properly compiled.

This is in preparation for creating a new webview, extract shared functionality to the webview-utils file

This module will behave like the interface.ts module and handle interactions between the compare webview and the extension.

Also, some moving around of functions and whitespace changes.

jcreedcmu · 2020-06-16T17:24:45Z

@dbartol about different ways to invoke comparison, I tend to agree with you that the most natural to me is select the two things and then invoke comparison --- but I think it's fine to get this PR in and then add that functionality afterwards, since multiselect brings a couple other complications with it. Showing an appropriate error message when any commands (including those other than comparison) are invoked with an inappropriate number of items selected is the main one that comes to mind.

* The error message will be displayed instead of the empty results tables. * Also, uncomment onEnterRules. That should never have been committed. * Also, extract CompareTable to its own component.

jcreedcmu · 2020-06-16T18:36:09Z

Looking good --- just one more suggestion, in CompareTable.tsx, how about adding a style={{verticalAlign: "top"}} to the first two <td>s in the table.vscode-codeql__compare-body ? When I compare two queries where the added rows greatly outnumber the deleted or vice-versa, the smaller one gets kinda visually lost by vertical centering.

Avoids an issue when one table has many more rows than the other and the tables are off-centered.

aeisenberg · 2020-06-16T18:57:38Z

Ping @github/product-docs-dsp!

This is a fairly large change. I can describe here how it will work, or we can sync up over zoom. Up to you.

Edit: I see you're already tracking this at: https://github.com/github/semmle-docs/issues/160

shati-patel · 2020-06-16T21:07:40Z

Thanks @aeisenberg, exciting update! Feel free to drop any information into the docs issue if you want, though we'll also let you know if we need more help when we get round to documentation 😃

aeisenberg marked this pull request as draft June 5, 2020 22:53

aeisenberg force-pushed the aeisenberg/compare branch from fcdbaa6 to 0531e4d Compare June 9, 2020 15:09

aeisenberg marked this pull request as ready for review June 10, 2020 16:00

aeisenberg requested review from jcreedcmu, adityasharad and dbartol and removed request for jcreedcmu and adityasharad June 10, 2020 16:00

aeisenberg force-pushed the aeisenberg/compare branch 2 times, most recently from e976db6 to ee8663b Compare June 15, 2020 21:28

jcreedcmu reviewed Jun 16, 2020

View reviewed changes

jcreedcmu suggested changes Jun 16, 2020

View reviewed changes

aeisenberg added 7 commits June 16, 2020 09:09

Introduce the skeleton compare view

07e116a

Adds the tsx file and updates the webpack config so that this new tsx file is properly compiled.

Extract shared components out of interface.ts

5f095ea

This is in preparation for creating a new webview, extract shared functionality to the webview-utils file

Add the compare-interface

33aa6fc

This module will behave like the interface.ts module and handle interactions between the compare webview and the extension.

More work on diffs

f250154

Add unit tests for interface-utils.ts

298f8ba

Also, some moving around of functions and whitespace changes.

Add link to open query results from compare view

8157327

Re-apply formatting to all files

735226c

aeisenberg force-pushed the aeisenberg/compare branch from ee8663b to 735226c Compare June 16, 2020 16:10

Display error message when can't compare queries

d063366

* The error message will be displayed instead of the empty results tables. * Also, uncomment onEnterRules. That should never have been committed. * Also, extract CompareTable to its own component.

aeisenberg added 2 commits June 16, 2020 11:48

Vertically align the two compare tables

64599de

Avoids an issue when one table has many more rows than the other and the tables are off-centered.

Update changelog

f69193e

jcreedcmu approved these changes Jun 16, 2020

View reviewed changes

aeisenberg merged commit 95988f0 into github:master Jun 16, 2020

aeisenberg deleted the aeisenberg/compare branch November 24, 2020 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compare view #431

Add compare view #431

aeisenberg commented Jun 5, 2020 •

edited

Loading

dbartol commented Jun 6, 2020

aeisenberg commented Jun 9, 2020 •

edited

Loading

dbartol commented Jun 10, 2020

aeisenberg commented Jun 10, 2020

aeisenberg commented Jun 15, 2020

jcreedcmu Jun 16, 2020

aeisenberg Jun 16, 2020

aeisenberg Jun 16, 2020

jcreedcmu left a comment

jcreedcmu Jun 16, 2020

aeisenberg Jun 16, 2020

jcreedcmu Jun 16, 2020

jcreedcmu Jun 16, 2020

aeisenberg Jun 16, 2020

jcreedcmu Jun 16, 2020

aeisenberg Jun 16, 2020

aeisenberg Jun 16, 2020

jcreedcmu Jun 16, 2020

aeisenberg Jun 16, 2020

aeisenberg Jun 16, 2020

jcreedcmu Jun 16, 2020

jcreedcmu commented Jun 16, 2020

jcreedcmu commented Jun 16, 2020

aeisenberg commented Jun 16, 2020 •

edited

Loading

shati-patel commented Jun 16, 2020

Add compare view #431

Add compare view #431

Conversation

aeisenberg commented Jun 5, 2020 • edited Loading

Checklist

dbartol commented Jun 6, 2020

aeisenberg commented Jun 9, 2020 • edited Loading

dbartol commented Jun 10, 2020

aeisenberg commented Jun 10, 2020

aeisenberg commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcreedcmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcreedcmu commented Jun 16, 2020

jcreedcmu commented Jun 16, 2020

aeisenberg commented Jun 16, 2020 • edited Loading

shati-patel commented Jun 16, 2020

aeisenberg commented Jun 5, 2020 •

edited

Loading

aeisenberg commented Jun 9, 2020 •

edited

Loading

aeisenberg commented Jun 16, 2020 •

edited

Loading