Add remote query items to history view #1155
Conversation
fb9e2c0 to
02838db
Compare
This is another incremental step on the way to saving history. This commit adds remote items to the history view. It adds in progress and completed icons. Users can explicitly remove items. Here is what is _not_ working: 1. Any other query history commands like open results or open query. 2. Seeing items after a restart.
02838db to
16df990
Compare
| return queryHistoryItem.initialInfo.queryText; | ||
| async getQueryText(item: QueryHistoryInfo): Promise<string> { | ||
| // TODO remote queries cannot have their text returned | ||
| return item.t === 'local' ? item.initialInfo.queryText : ''; |
There was a problem hiding this comment.
FYI we store the queryText in the RemoteQuery entity.
There was a problem hiding this comment.
Let me change the TODO text. The queryText is just not yet available.
| @@ -61,6 +65,13 @@ export class RemoteQueriesManager { | ||
| query: RemoteQuery, | ||
| cancellationToken: CancellationToken | ||
| ): Promise<void> { | ||
| const queryId = this.createQueryId(query.queryName); |
There was a problem hiding this comment.
It probably doesn't make a huge difference pragmatically at the moment, but to me it feels like the logical place for this code (generating query id, preparing storage and creating a history item) is runRemoteQuery rather than monitorRemoteQuery.
Also if we do that, monitorRemoteQuery can be called when we rehydrate query history after a VS Code restart.
There was a problem hiding this comment.
Yes...the next PR will be about refactoring how we do monitoring and storing the query. I'd like to centralize the object that (de-)serializes query data, so this can happen outside of the initial running of the query.
| await this.interfaceManager.showResults(query, queryResult); | ||
| } | ||
| // Ask if the user wants to open the results in the background. | ||
| void this.askToOpenResults(query, queryResult).then( |
There was a problem hiding this comment.
Any reason we're not doing await with try/catch?
There was a problem hiding this comment.
I want this to happen async, in the background.
There was a problem hiding this comment.
Ah good point. Should we remove await from the other branches too? Doesn't have to be in this PR, just curious.
There was a problem hiding this comment.
Yes. I think that makes sense.
380a203 to
90764ae
Compare
…/remote-query-save
90764ae to
9c6ae22
Compare
| await this.interfaceManager.showResults(query, queryResult); | ||
| } | ||
| // Ask if the user wants to open the results in the background. | ||
| void this.askToOpenResults(query, queryResult).then( |
There was a problem hiding this comment.
Ah good point. Should we remove await from the other branches too? Doesn't have to be in this PR, just curious.
|
Going to hold off merging this PR until the next (not yet created PR) is approved and can be merged at the same time. This PR does what it says, but leaves the extension in a bit of an awkward state:
The next PR (coming soon) will remove these two restrictions. |
This is another incremental step on the way to saving history.
This commit adds remote items to the history view. It adds in progress
and completed icons. Users can explicitly remove items.
Here is what is not working:
To make further progress, there will need to be another bit of refactoring where we separate monitoring and opening remote queries from the remote-query manager. There needs to be a way for the query history manager to kick off the monitoring process for any in-progress history item.
Checklist
ready-for-doc-reviewlabel there.