Allow disposable pattern on TreeItems

[eamodio]

This is tangentially related to #30535.

As mentioned, in GitLens and in the upcoming CodeStream extension, I have tree items that have event subscriptions based on their expanded state (or in some cases just existence -- i.e. visibility). Currently there is no good way to clean up these subscriptions if the items get re-rendered (i.e. replaced with a new instance -- because I am not maintaining a duplicate set of TreeItem that the tree already maintains) or removed from the tree (maybe by a far upstream ancestor).

It would be great if upon removal/clearing of the tree items, their dispose method would be called if one exists. That way any subscriptions could be appropriately removed.

Likely here:
https://github.com/Microsoft/vscode/blob/b8df1293c13dfc75c9cff388a0a0994475441198/src/vs/workbench/api/node/extHostTreeViews.ts#L407

And here:
https://github.com/Microsoft/vscode/blob/b8df1293c13dfc75c9cff388a0a0994475441198/src/vs/workbench/api/node/extHostTreeViews.ts#L421

//cc @sandy081

[eamodio]

@sandy081 Is this something that a PR would likely be accepted for?

[sandy081]

Sure. But it needs some thinking about when an item has to be disposed.

[eamodio]

@sandy081 maybe adding something like you just added in #30535 would work

/**
 * Event that is fired when an element is removed
 */
readonly onDidRemoveElement: Event<T>;

Thoughts?

[sandy081]

@eamodio Were you not aware when an element is removed as you are the one that provides elements? Its the extension that triggers if there is a change like addition or removal or other changes. So I am guessing extension should be aware when an element is removed right?

[eamodio]

While I provide the elements, I am not maintaining a collection of them (since vscode is already doing that itself). It makes things easier since I can drop whole sections of the tree on an ancestor refresh and not have to worry about it. Which in most cases is just fine, but if some of the items have subscriptions, they need to get cleaned up, which is why I would like an event (or a dispose pattern) for when an item is removed from the tree.

[sandy081]

So when you said you drop the whole sections of the tree... it means you have a tree data model of elements on your side? As my understanding, if an extension is triggering a change event, then it should be storing the elements map. Also, extension is the main source to know when elements are added and removed. Hence it owns the onChange event. Exposing onDispose event from tree side looks to me wrong ownership.

[eamodio]

I don't have a data model of the tree elements in the extension. I build all the tree elements on demand. So when I say dropping sections of the tree I mean like the following: On most nodes (that can have children), I add a refresh command (to allow the user to refresh a branch/tag/stash/etc and have it update all its children/descendants). The refresh command just sends the onDidChangeTreeData for that node, which causes a new tree item to get created in the place of the one being refreshed -- so if the existing node had subscriptions (or any of its descendants) they are orphaned but still subscribed. Since I am not maintaining a duplicate data model of the tree, I don't even know which elements would be removed, so I have no good way of cleaning them up.

Does that make sense?

[sandy081]

I see, your node does not know about its descendants.. you always create them on fly when asked for the children? And subscriptions are attached to the node during getChildren call?

[eamodio]

Correct. Some are subscribed on getChildren() some are on getTreeItem

[vscodenpa]

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!