Skip to content

Commit

Permalink
Fix #127621
Browse files Browse the repository at this point in the history
  • Loading branch information
@roblourens
roblourens committed Jun 30, 2021
1 parent 5dd14d0 commit 07a0575
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions src/vs/workbench/contrib/search/browser/searchView.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,25 +510,26 @@ export class SearchView extends ViewPane {
}

refreshTree(event?: IChangeEvent): void {
const setChildrenOpts = { diffIdentityProvider: { getId(element: RenderableMatch) { return element.id; } } };

This comment has been minimized.

Sign in to view

Copy link
@jrieken

jrieken Jul 1, 2021

Member

@joaomoreno Is there any reason why the tree isn't picking up an existing identity provider? E.g this tree already defines one here and this issue wouldn't have happened

This comment has been minimized.

Sign in to view

Copy link
@joaomoreno

joaomoreno Jul 1, 2021

Member

When @connor4312 introduced the diff identity provider, we decided to keep it separate and opt-in, until we have enough coverage. We can certainly enable it by default, but it definitely requires planning time for it, given one person (me) will be possible breaking lists and trees everywhere. 🙀

This comment has been minimized.

Sign in to view

Copy link
@jrieken

jrieken Jul 1, 2021

Member

Been there too many times? Maybe a good start would be a ctor-option that enables this. Would be more obvious than the setChildren-call

This comment has been minimized.

Sign in to view

Copy link
@roblourens

roblourens Jul 1, 2021

Author Member

This failure was a good example of why we should be careful, my id is not quite unique enough for parent nodes.

This comment has been minimized.

Sign in to view

Copy link
@connor4312

connor4312 Jul 1, 2021 •
edited
Loading

Member

The main incompatibility is that setChildren will no longer trigger a re-render of same-ID nodes subtree. If a consumer depends on this happening, they'll need some kind of migration. This seems to be the case in a decent number of locations, e.g. when Isidor adopted it for the debug REPL -- and these can be somewhat subtle #116043
const collapseResults = this.searchConfig.collapseResults;
if (!event || event.added || event.removed) {
// Refresh whole tree
if (this.searchConfig.sortOrder === SearchSortOrder.Modified) {
// Ensure all matches have retrieved their file stat
this.retrieveFileStats()
.then(() => this.tree.setChildren(null, this.createResultIterator(collapseResults)));
.then(() => this.tree.setChildren(null, this.createResultIterator(collapseResults), setChildrenOpts));
} else {
this.tree.setChildren(null, this.createResultIterator(collapseResults));
this.tree.setChildren(null, this.createResultIterator(collapseResults), setChildrenOpts);
}
} else {
// If updated counts affect our search order, re-sort the view.
if (this.searchConfig.sortOrder === SearchSortOrder.CountAscending ||
this.searchConfig.sortOrder === SearchSortOrder.CountDescending) {
this.tree.setChildren(null, this.createResultIterator(collapseResults));
this.tree.setChildren(null, this.createResultIterator(collapseResults), setChildrenOpts);
} else {
// FileMatch modified, refresh those elements
event.elements.forEach(element => {
this.tree.setChildren(element, this.createIterator(element, collapseResults));
this.tree.setChildren(element, this.createIterator(element, collapseResults), setChildrenOpts);
this.tree.rerender(element);
});
}
Expand Down

0 comments on commit 07a0575

Please sign in to comment.