Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix set label command on history items #710

Merged
merged 2 commits into from
Dec 16, 2020

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 :)

@@ -157,8 +144,8 @@ class HistoryTreeDataProvider extends DisposableObject implements QueryHistoryDa
return this.history;
}

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