Skip to content

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Dec 15, 2020

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.

Fixes #709.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@aeisenberg aeisenberg marked this pull request as draft December 15, 2020 23:36
@aeisenberg
Copy link
Contributor Author

Converting to draft since I want to add a few unit tests.

Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks very reasonable. I'll wait for tests :)


refresh() {
this._onDidChangeTreeData.fire(undefined);
refresh(CompletedQuery?: CompletedQuery) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase the variable to distinguish them.

@aeisenberg aeisenberg marked this pull request as ready for review December 16, 2020 00:24
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.
@aeisenberg aeisenberg merged commit e6eb914 into github:main Dec 16, 2020
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.

Set Label on a query history item is broken

2 participants