Fix "View SARIF" context menu race. #598
Conversation
| @@ -77,6 +86,9 @@ class HistoryTreeDataProvider | |||
| constructor(private ctx: ExtensionContext) { } | |||
|
|
|||
| async getTreeItem(element: CompletedQuery): Promise<vscode.TreeItem> { | |||
| if (element.treeItem !== undefined) | |||
There was a problem hiding this comment.
I think it would be nicer to convert treeItem to a getter on CompletedQuery. And move all this code into the getter. It avoids a type assertion on updateTreeItemContextValue.
There was a problem hiding this comment.
Wait, no, this function needs to have the context of the treeview. The CompletedQuery doesn't have that information, or at least doesn't plausibly have it until the treeitem is created.
| @@ -50,6 +50,15 @@ const SHOW_QUERY_TEXT_QUICK_EVAL_MSG = `\ | |||
| */ | |||
| const FAILED_QUERY_HISTORY_ITEM_ICON = 'media/red-x.svg'; | |||
|
|
|||
| export async function updateTreeItemContextValue(element: CompletedQuery): Promise<void> { | |||
There was a problem hiding this comment.
Would this make more sense as a member function on CompletedQuery?
There was a problem hiding this comment.
I think it makes sense as a member function on QueryHistoryManager, because of my attempting to decouple CompletedQuery from having to know too much about the whole QueryHistoryManager and the associated tree data provider. Already I don't love having to put the tree item itself as a field in CompletedQuery, but this is the simplest thing that I can see works.
|
I tried testing this and it isn't working yet for me. |
|
I had forgotten to actually |
This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes github#598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation.
This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes github#598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation.
This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes github#598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation.
This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes #598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation.
This removes the cached treeItem that is a property of the completedQuery. We should not be caching them since they are cached by the vscode api itself. Instead, we should recreate whenever requested. Also, this change fixes github#598 in a new way. Instead of adding the context to the cached treeItem, we simply refresh only the item that has changed. This is a fast operation.
Fixes #598.