Only call refresh on given elements given by plugin #14697
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes eclipse-theia#14390 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>
msujew
approved these changes
Jan 7, 2025
| @@ -428,6 +428,18 @@ export class PluginTreeModel extends TreeModelImpl { | |||
|
|
|||
| @injectable() | |||
| export class TreeViewWidget extends TreeViewWelcomeWidget { | |||
| async refresh(items: string[] | undefined): Promise<void> { | |||
There was a problem hiding this comment.
Minor: It might make more sense to use ? here, as I would much rather call tree.refresh() than tree.refresh(undefined).
Suggested change
| async refresh(items: string[] | undefined): Promise<void> { | |
| async refresh(items?: string[]): Promise<void> { |
Comment on lines
+261
to
+267
| for (const element of elements) { | ||
| for (const node of this.nodes.values()) { | ||
| if (node.value === element) { | ||
| ids.push(node.id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: I'm not sure how bad this can get in reality, but this is O(n * m) behavior. We can cut this down to O(n + m).
Suggested change
| for (const element of elements) { | |
| for (const node of this.nodes.values()) { | |
| if (node.value === element) { | |
| ids.push(node.id); | |
| } | |
| } | |
| } | |
| const set = new Set<T>(); | |
| for (const element of elements) { | |
| set.add(element); | |
| } | |
| for (const node of this.nodes.values()) { | |
| if (set.has(node.value)) { | |
| ids.push(node.id); | |
| } | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it does
In the scenario from the issue, the extensions calls
onDidChangeTreeData?: [Event]<void | T | T[]>on some elements. However, Theia disregards the elements to be refreshed and always refreshes from the root. Since theTreeIteminstances furnished by the extensions seem to be new each time, we cannot associate the refreshed items with cached tree nodes and therefore we lose the expansion state: for us, it looks like we have a completely new tree that starts out collapsed.This PR implements the refresh of only a given elemement. This prevents the refresh from the root and thus fixes the problem.
Fixes #14390
How to test
Make sure other contributed views still work the same as before, for example views from gitlens, reference, etc.
Follow-ups
Breaking changes
Attribution
Contributed on behalf of STMicroelectronics
Review checklist
Reminder for reviewers