Skip to content

Commit

Permalink
update collapsible when children change
Browse files Browse the repository at this point in the history
  • Loading branch information
@connor4312
connor4312 committed Jan 14, 2021
1 parent 9b83eb6 commit a8dd7f6
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/vs/base/browser/ui/tree/indexTreeModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ export class IndexTreeModel<T extends Exclude<any, undefined>, TFilterData = voi
const nodesToInsertIterator = Iterable.map(toInsert, el => this.createTreeNode(el, parentNode, parentNode.visible ? TreeVisibility.Visible : TreeVisibility.Hidden, revealed, treeListElementsToInsert, onDidCreateNode));

const lastIndex = location[location.length - 1];
const lastHadChildren = parentNode.children.length > 0;

// figure out what's the visible child start index right before the
// splice point
Expand Down Expand Up @@ -296,6 +297,11 @@ export class IndexTreeModel<T extends Exclude<any, undefined>, TFilterData = voi
deletedNodes.forEach(visit);
}

const currentlyHasChildren = parentNode.children.length > 0;
if (lastHadChildren !== currentlyHasChildren) {
this.setCollapsible(location.slice(0, -1), currentlyHasChildren);
}

this._onDidSplice.fire({ insertedNodes: nodesToInsert, deletedNodes });

let node: IIndexTreeNode<T, TFilterData> | undefined = parentNode;
Expand Down
24 changes: 24 additions & 0 deletions src/vs/base/test/browser/ui/tree/indexTreeModel.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,30 @@ suite('IndexTreeModel', () => {
assert.deepEqual(list[2].depth, 1);
}));

test('updates collapsible', () => withSmartSplice(options => {
const list: ITreeNode<number>[] = [];
const model = new IndexTreeModel<number>('test', toList(list), -1);

model.splice([0], 0, [
{
element: 0, children: [
{ element: 1 },
]
},
], options);

assert.strictEqual(list[0].collapsible, true);
assert.strictEqual(list[1].collapsible, false);

model.splice([0, 0], 1, [], options);
assert.strictEqual(list[0].collapsible, false);
assert.strictEqual(list[1], undefined);

model.splice([0, 0], 0, [{ element: 1 }], options);
assert.strictEqual(list[0].collapsible, true);
assert.strictEqual(list[1].collapsible, false);
}));

test('expand', () => withSmartSplice(options => {
const list: ITreeNode<number>[] = [];
const model = new IndexTreeModel<number>('test', toList(list), -1);
Expand Down

5 comments on commit a8dd7f6

@joaomoreno
Copy link
Member

Choose a reason for hiding this comment

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

@connor4312 With this change, the index tree started making decisions about the collapsible state, given the number of children an element may have. But an element might still want to be collapsible and just have zero children... This is the cause for the twistie to disappear in #199441 (comment).

Do you still remember what made you make this change here deep down? I'd love to revert but keep whatever behavior you intended in the first place, probably around the test tree.

@connor4312
Copy link
Member Author

@connor4312 connor4312 commented on a8dd7f6 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this was because the parent of a tree element is generally, with the diffIdentityProvider, no longer re-rendered when its children are updated, so my guess is it was leaving twisties where they should no longer have been left.

...but I also don't observe any negative effects when commenting out this line of code in the test explorer which uses the diffIdentityProvider heavily 🤷 Feel free to revert, we have two months to find any resulting issues

@joaomoreno
Copy link
Member

Choose a reason for hiding this comment

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

I've reverted it: #201789
Related to: #199441 (comment)

Let's keep an eye on the test explorer. 👌

@connor4312
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this caused #204805. Will try for a fix...

@connor4312
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the fix for that is just to have the test code update the collapsible state of parent items when they add children. This is a weird case and it seems like trees should 'just handle' that case but I see the argument that the responsibility of collapsibility should lie solely with the consumer

Please sign in to comment.