Skip to content

Conversation

jcreedcmu
Copy link
Contributor

@jcreedcmu jcreedcmu commented Feb 20, 2020

Add query history items right when the user initiates a query, rather
than waiting until query compilation/execution has finished. This
leads to a more predictable ordering of query history items.

This doesn't affect user-facing behavior other than timing.

Fixes https://github.com/github/codeql-coreql-team/issues/277

Add query history items right when the user initiates a query, rather
than waiting until query compilation/execution has finished. This
leads to a more predictable ordering of query history items.
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

My main concern with this PR relates to the synchronisation of the selected item in the query history view with the query results view. In other words, I think the query results view should always show the results for the selected query history item.

This is already to some extent an existing issue, since queries with errors can be selected. However adding query history items for in-progress queries makes this a worse experience since:

  • There is no visual indication that a query history item is in-progress, so it's not obvious that the results view is showing the results of a different query history item.
  • The in-progress history item is automatically selected as soon as the query starts running, so it's far more likely the history view and results view will be out of sync.

Proposal:

  • When a query run is started, an in-progress query history item is added to the query history list but is not automatically selected.
  • When a query run finishes, if it is the most recent query run in the query history list, the corresponding query history item is selected, and the results view is updated with the results of that query.
  • It is not possible to select in-progress or errored query history items.
  • The results view is always synced with the selected query history item.
  • The results view always shows a CompletedQuery, i.e. a query with results.

@@ -94,8 +102,8 @@ export class CompletedQuery implements QueryWithResults {
return this.config.format;
}

get didRunSuccessfully(): boolean {
return this.result.resultType === messages.QueryResultType.SUCCESS;
get hadErrors(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest renaming to didFinishWithErrors or similar to be reinforce that this query could still be running.

@@ -261,7 +261,7 @@ async function activateWithInstalledDistribution(ctx: ExtensionContext, distribu
ctx.subscriptions.push(intm);
archiveFilesystemProvider.activate(ctx);

async function showResultsForCompletedQuery(query: CompletedQuery, forceReveal: WebviewReveal): Promise<void> {
async function showResultsForCompletedQuery(query: RunningOrCompletedQuery, forceReveal: WebviewReveal): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to reflect that the query may not be completed, or reintroduce the CompletedQuery type as a refinement of RunningOrCompletedQuery

@jcreedcmu
Copy link
Contributor Author

The statement

There is no visual indication that a query history item is in-progress
isn't quite true --- I did add the string "Running" in the query history view itself, though it might not be visible depending on the size the user has chosen for that side panel. Otherwise you're absolutely correct about the results view getting out of sync.

I think it might be simpler if the results view actually presents some visual that conveys 'this query terminated with an error' (maybe even showing a useful error message) or a spinner animation or something to indicate that the query is still running, if it is? Selecting the item for the just-started query (before it has finished) makes sense to me, if the results view changes from 'still running spinner' to the actual results when the query finishes.

@henrymercer
Copy link
Contributor

For the use case of progressively refining a QL query, I think continuing to show the previous results until the new results are available (instead of displaying a spinner in the results view as soon as the new query run is started) would be valuable.

@jcreedcmu
Copy link
Contributor Author

Closing for now, need to rethink approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants