Skip to content

fix issue with finding required node handles #118091

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

Merged
merged 1 commit into from
Mar 31, 2021
Merged

fix issue with finding required node handles #118091

merged 1 commit into from
Mar 31, 2021

Conversation

alanrenmsft
Copy link
Contributor

This PR fixes #microsoft/azuredatastudio#14229

when the elements parameter have leaf nodes then parent nodes, this method won't work and instead of only returning the parent node, it will also returned the leaf nodes. for example, the elements contains: '1/2' (2 is a child of 1) and '1', instead of returning '1' it will return both nodes.

@alanrenmsft
Copy link
Contributor Author

@weinand could you please take a look?

@weinand weinand removed their assignment Mar 26, 2021
@sandy081 sandy081 assigned alexr00 and unassigned sandy081 Mar 29, 2021
@alexr00
Copy link
Member

alexr00 commented Mar 29, 2021

@alanrenmsft is there a way I can repro the issue with VS Code?

@alanrenmsft
Copy link
Contributor Author

@alexr00 I created an extension to reproduce the issue: https://github.com/alanrenmsft/test_tree

  1. you can find the vsix test-tree-0.0.6.vsix in the repo and download it.
  2. launch your local vscode dev instance and install the vsix and reload the dev instance
  3. after it is loaded, you will see a view under File Explorer 'MY TREE VIEW' and make sure you expand all the nodes under it
    image
  4. now make sure Extension host is attached and open the file: src\vs\workbench\api\common\extHostTreeViews.ts and set breakpoint at the first line of getHandlesToRefresh method
  5. Run the command "Refresh Test Tree" from command palette, you can ignore the first hit of the breakpoint and the second hit, you should see there are 2 items in the elements parameter. (the command will first refreshing node at level 4 and then refresh node at level 4 and node at level 2)
  6. put a breakpoint at the end of the method and observe the value being returned.
    image

expected: it should only return the node at level 2, but instead it is returning both nodes.
7. let it continue execution and you will see:
image

you can apply my changes locally and test it again, it should fix the problem.

@@ -497,12 +497,12 @@ class ExtHostTreeView<T> extends Disposable {

private getHandlesToRefresh(elements: T[]): TreeItemHandle[] {
const elementsToUpdate = new Set<TreeItemHandle>();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Very good change. Thank you for the detailed steps and sample to repro with!

@alexr00 alexr00 added this to the April 2021 milestone Mar 31, 2021
@alexr00 alexr00 merged commit 1d883f8 into microsoft:main Mar 31, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants