Introduce helpers for commands that operate on selections#2287
Introduce helpers for commands that operate on selections#2287robertbrignull merged 30 commits intomainfrom
Conversation
a28875f to
429ce43
Compare
429ce43 to
14952cf
Compare
aeisenberg
left a comment
There was a problem hiding this comment.
I really like this solution.
| // A hack to match types that are not an array, which is useful to help avoid | ||
| // misusing createSingleSelectionCommand, e.g. where T accidentally gets instantiated | ||
| // as DatabaseItem[] instead of DatabaseItem. | ||
| type NotArray = object & { length?: never }; |
| type SelectionCommand<T extends NotArray> = CreateSupertypeOf< | ||
| TreeViewContextMultiSelectionCommandFunction<T> & | ||
| TreeViewContextSingleSelectionCommandFunction<T> & | ||
| ExplorerSelectionCommandFunction<T>, | ||
| (singleItem: T, multiSelect?: T[] | undefined) => Promise<void> | ||
| >; |
There was a problem hiding this comment.
This is some great typescript-fu.
| if (multiSelect !== undefined && multiSelect.length > 0) { | ||
| return f(multiSelect); | ||
| } else { | ||
| return f([singleItem]); |
There was a problem hiding this comment.
Am I correct in understanding that if singleItem is undefined, we will be returning f([undefined]) here?
There was a problem hiding this comment.
As per the types and my experimentation, singleItem cannot be undefined. If you can find a way to trigger one of our commands and get singleItem to be undefined I'd be very interested to hear. Otherwise I'd prefer to keep our types and code simple.
There was a problem hiding this comment.
We seem to be checking whether a databaseItem is undefined in three places?
40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R518
40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R540
40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R564
There was a problem hiding this comment.
Thanks for pointing those out. The commands around upgrading databases are a little complicated because we kinda have two notions of being "selected". You can click to highlight entries in the list (i.e. the vscode tree view), and this state gets passed through to the codeQLDatabases.upgradeDatabase command if you select to upgrade the databases from the context menu. You can also click "make selected" on a database and this state is managed internally, and this is what is used for the codeQL.upgradeCurrentDatabase or codeQL.runQuery (a.k.a. "Run Query on Selected Database") commands.
Looking at each bit of code:
- 40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R518
- As mentioned above, the
codeQL.upgradeCurrentDatabasecommand isn't really a "selection command" like the ones we're looking at in this PR. It's operating on the currently selected local database, but it's not a vscode tree view selection like the other cases in this PR. It's a selection that we manage ourselves, and thus you can trigger this command at any time to operate on the "current" database, and there may be a database selected or not.
- As mentioned above, the
- 40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R540
- This commit is from quite early in the PR and is doing some refactoring before we get into the rest of the PR. The code being linked to is already changed on the latest commit.
- 40f3092#diff-b11b6f7e1d76e818f721c94ae740f046663c80d519c2f515f2f78344f90e6c78R564
- Thanks. This check is unnecessary. It's an oversight from me just wrapping the existing function implementation with
await Promise.all. I've pushed a commit to remove this check.
- Thanks. This check is unnecessary. It's an oversight from me just wrapping the existing function implementation with
| queryHistoryManager.treeDataProvider.getCurrent(), | ||
| ).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Has the behaviour changed or is there a different reason for removing this test?
I can see we've removed some of the tests that were checking for multiple items being passed, when the function should only receive a single item. I think for those cases, it makes sense for the tests to become redundant.
There was a problem hiding this comment.
I'm deleting this test because it was covering the case of having multiple items selected. I don't quite understand why it's named "double click", but in practice it's calling handleItemClicked as if two items in the query history are selected, and then asserting it ignores it and does nothing. It's now no longer possible to pass two items to handleItemClicked, so we don't need this test anymore.
There was a problem hiding this comment.
It's now no longer possible to pass two items to handleItemClicked, so we don't need this test anymore.
Ah, I see. Thanks for explaining!
There was a problem hiding this comment.
I think the reason we added the test in the first place was because double clicking was causing problems and we fixed the issue. We wanted to make sure we account for the behaviour. It was kind of a superfluous test if we were already testing this in other places. Perhaps it was me not having a proper overview of the existing tests when this was introduced. Anyway, I agree we don't need this.
Conflicts: extensions/ql-vscode/src/databases/local-databases-ui.ts extensions/ql-vscode/src/query-history/query-history-manager.ts
This PR aims to improve our commands that work on selections of items from the UI. These commands all follow a pattern but currently there's nothing enforcing the arguments the commands take or how they process them. I've added new helper functions and converted all of the commands to use the helpers.
This should hopefully help in the following ways:
I'm still open for suggestions of how to make this even better. We could definitely reduce duplication even more, or make things more streamlined or safer.
Checklist
ready-for-doc-reviewlabel there.