Display different message when queries are loading vs no queries found#2523
Display different message when queries are loading vs no queries found#2523robertbrignull merged 9 commits intomainfrom
Conversation
|
Screen recording: Screen.Recording.2023-06-19.at.12.30.47.movUnfortunately it still displays the welcome message for a bit while the extension is starting, before it calls out tree data provider which can provider the loading message. I'm considering flipping the behaviours so that we use the welcome message to say that things are in progress, and then a custom message when there were no queries found. |
robertbrignull
force-pushed
the
robertbrignull/queries-loading
branch
from
383a37e to
89749ff
Compare
robertbrignull
force-pushed
the
robertbrignull/queries-loading
branch
from
89749ff to
d290b56
Compare
robertbrignull
changed the title
I've updated this PR as described above. I've updated the PR description too. Should be ready for review and testing now. |
| import { createMultiSelectionCommand } from "../common/vscode/selection-commands"; | ||
| import { findLanguage } from "../codeql-cli/query-language"; | ||
| import type { QueryTreeViewItem } from "../queries-panel/query-tree-view-item"; | ||
| import { QueryTreeViewItem } from "../queries-panel/query-tree-view-item"; |
There was a problem hiding this comment.
Is there a reason you removed the type import specification?
There was a problem hiding this comment.
Good spot. Earlier in the lifetime of this PR I had to convert the type import because I used QueryTreeViewItem in a instanceof expression, and that requires importing the type at runtime instead of just at compiletime. But that has been removed now so this can stay as a type import.
| public getLanguageForQueryFile(queryPath: string): QueryLanguage | undefined { | ||
| // Find all packs in a higher directory than the query | ||
| const packs = this.getPathData().filter((queryPack) => | ||
| const packs = (this.getPathData() || []).filter((queryPack) => |
There was a problem hiding this comment.
It's a bit confusing for me to run a filter over an empty array, but checking getPathData first and having another if condition is not really better I suppose?
There was a problem hiding this comment.
We could check it beforehand. It makes it longer and more verbose, but that's not necessarily a bad thing. I'll make it a separate check instead of using || [] inside this expression.
| } | ||
| } | ||
|
|
||
| export function createQueryTreeNodeItem( |
There was a problem hiding this comment.
I think I would personally prefer using the domain names here, namely QueryTreeFolderItem and QueryTreeFileItem. Right now I have to translate 'node' and 'leaf' in my head to folder and file to have the correct image in my head. However, I don't have a CS background and could imagine it doesn't make sense to call it something other than node and leaf? For me folder and file is just more descriptive.
There was a problem hiding this comment.
I don't have a strong preference between the names. I was just picking one and my mind goes towards nodes and leaves because we're making TreeView/QueryTree.
I do now see we're using "queryFolder" and "queryFile" for the context values. I'll change the function names to match those.
norascheuch
left a comment
norascheuch
left a comment
There was a problem hiding this comment.
Some minor comments otherwise looks good. Thanks for introducing these types!
2 participants
Displays a "loading" message while query discovery has not yet finished, and a separate message when no queries have been found. This should help avoid confusing users by saying there are no queries when really the discovery is just in progress.
The approach I've taken is to make the welcome message the "loading" message, and display a custom text node for the "no queries" case. This helps avoid the effect seen in #2523 (comment) where it flips between the two options before the extension has started initialising.
For the datatypes I opt to create subclasses for
QueryTreeQueryItemandQueryTreeTextItem, whereQueryTreeQueryItemis identical to the oldQueryTreeViewItem. We could maybe split this up further into "node" and "leaf" classes, but I haven't done this here.I also fix a bug where it wouldn't update the tree view in the case that the initial refresh found no queries. This bug just wasn't visible before because it would display the welcome message anyway.
Checklist
ready-for-doc-reviewlabel there.