Selection and expansion state is lost after tree node label change

[weinand]

The custom explorer's selection and expansion state is only retained, if the label of the tree node does not change.
If the label changes (e.g. because one part of it reflects a dynamically changing status), the selection and expansion state is lost.

Please note that in my TreeDataProvider implementation I'm returning the same TreeItem instance for the same tree node, so I'm not deleting a TreeItem and I'm not creating a new in its place. In this case the VS Code custom tree should have no problem to preserve the selection because the TreeNode instance does not change (only its label). It should not use the label as a unique ID but assign a unique ID to the TreeItems on its own.

And even when using a TreeDataProvider implementation that returns new TreeItems on every refresh it should be possible that the selection is preserved. For this the TreeItem could support an id attribute that could be overwritten if necessary.

Since the lost selection makes a dynamic tree very difficult to use for users, I think this is an "important" bug.

[weinand]

After discussions with @sandy081 I tried this workaround:

Instead of implementing a TreeDataProvider<TreeItem> I now use a TreeDataProvider<string>.

In the former case VS Code uses the label property of a TreeItem as the ID of the tree node (which of course is not stable if the label is modified).
In the latter case VS Code uses the template parameter as the ID of the tree node. Since this ID is independent from the tree node's label, it is robust. However, now I have to manage the IDs and maintain a mapping between ID and TreeItems.

With this approach selection and expansion state was preserved after tree item changes.

Since this workaround exists, I've removed the "Important" label.

However, this workaround uncovered another bug #40179.

[sandy081]

After discussions with @weinand following is implemented

Make sure UI state of a node is retained until the handle is not changed. A tree provider provides handles and resolve the handle to TreeItem. A handle can be of any type, it can be a string or TreeItem itself. Handles are identifiers for TreeItems/Nodes.

Another implementation detail is that, ID of a TreeItem node is generated from its parent id + index + initial_label. This will help in retaining the UI state of nodes when authors do not want preserve handles.

[eamodio]

So a tree provider implementation will be able to supply the handle to be used for a specific tree item?

[sandy081]

@eamodio It already exists. I refer Element of type T in the provider as handle in my previous comment.

export interface TreeDataProvider<T> {
		/**
		 * An optional event to signal that an element or root has changed.
		 * To signal that root has changed, do not pass any argument or pass `undefined` or `null`.
		 */
		onDidChangeTreeData?: Event<T | undefined | null>;

		/**
		 * Get [TreeItem](#TreeItem) representation of the `element`
		 *
		 * @param element The element for which [TreeItem](#TreeItem) representation is asked for.
		 * @return [TreeItem](#TreeItem) representation of the element
		 */
		getTreeItem(element: T): TreeItem | Thenable<TreeItem>;

		/**
		 * Get the children of `element` or root if no element is passed.
		 *
		 * @param element The element from which the provider gets children. Can be `undefined`.
		 * @return Children of `element` or root if no element is passed.
		 */
		getChildren(element?: T): ProviderResult<T[]>;
	}

It is up to Tree provider to decide what kind of handle it wants to provide. It can be a string or TreeItem itself or any object. It is important that the provider resolves given element/handle to a TreeItem.

Basically, there is no change in the API and the contract. The underlying implementation got changed to retain the state of an element/handle until it is not changed.

[eamodio]

Ah, so you can't use a TreeItem and provide a handle or id on it.

[weinand]

I find the statement "to retain the state of an element/handle until it is not changed" confusing because it still sounds like the state is lost if the element/handle changes.

I think it should read: "the UI preserves the selection and expansion state for a tree node based on the node's handle. If the handle disappears the state is lost."

In addition I do not understand why you mention how the internal handle "ID" is calculated. First I think the calculation is wrong because it takes the child index into account which ties the retained state to a child index and not to a child identity. And secondly I do not understand how this calculation could help authors in any way.

[eamodio]

I think it just speaks to the stability (or instability) of how the state will be preserved.

Personally I would like to be able to still use TreeItem (I believe it will cause be a great deal of work to change it), but to be able to provide an identifier to be using in place of the label in any true id calculation -- something like TreeItem.key or TreeItem.handle -- which could just be used in your ID calculation by parent id + index + (TreeItem.handle || initial_label)

But it is totally optional and no behavior changes if it is not there.

For example, for most things in GitLens, using the label of a TreeItem works perfectly well, but for a couple of items, the label contains a count or can be changed. So just for those couple of items, I would like to use something like TreeItem.handle above.

[weinand]

You can continue to use the TreeItem as the handle. The only thing that has changed is that the ID of the TreeItem is not based on its label anymore. So you cannot change the ID of a TreeItem.

[eamodio]

Oh, so state will be preserved even if the label changes. Got it. Thanks!

[sandy081]

@weinand Thanks for the wordings and for making it clear.

• Yes, state is not dependent on the label
• Yes, TreeItem can be used as the handle for itself

In addition I do not understand why you mention how the internal handle "ID" is calculated. First I think the calculation is wrong because it takes the child index into account which ties the retained state to a child index and not to a child identity.

I mentioned that as just as an implementation detail. This should not impact the above behaviour as ID remains same always for the given handle. But, I see including index might be an issue if the children are shuffled. I will look into that.

And secondly I do not understand how this calculation could help authors in any way.

Advantage it has is when the extension authors create new handles for the same TreeItem then this tries to preserves the selection and expansion state which I thought is good.

Thanks.

[weinand]

Trying to match new handles to old handles (based on the TreeItem) is wrong because it takes away control from the TreeDataProvider and makes the whole story non-deterministic. How do you detect that the new handle should take over the retained state from the old handle? Just because the new handle sits at the same position as the old handle?

I firmly believe that the underlying model should be only this:

• Every handle gets a unmodifiable unique ID on creation.
• selection and expansion state is retained based on those IDs.

[sandy081]

@weinand Yes, the disadvantage of that approach is causing non-deterministic. It's good to have deterministic behaviour. I will change the implementation to have a unmodifiable unique ID per handle.

[eamodio]

Wait, so now with this change when using a TreeItem there is now no way to preserve the state without keeping the reference to the same TreeItem? At least with using the initial label, we would be able to preserve state for something that was "effectively" the same thing. With this change, GitLens will now lose almost all of its TreeItem state -- because I don't persist anything but the root nodes, and re-generate all the other nodes on refresh (it would be quite expensive for me to keep the same list of TreeNodes around that vscode is already doing in the list).

I feel strongly now that with this change there should be a way to an extension to provide that unique id (still can be unmodifiable and at creation of the TreeItem), but without some sort of extension provided correlation key (label or specifically provided id) this is a HUGE step backward.

[sandy081]

@eamodio Right, I was using the initial label and index to generate an ID for the TreeItem. But this could lead to non-deterministic behaviour if the label or position has changed.

The right solution for this is to have an optional id property on TreeItem that can be used to generate real ID. This will help extensions not to persist handles.

[weinand]

@sandy081 with your proposal to introduce an 'id' attribute are the following statements correct?

• for backward compatibility the ID of a TreeItem (aka handle) is calculated like before (based on 'label' and 'index' attributes).
• if a new attribute 'id' is set, the ID is using that (and 'label' and 'index' is no longer used to calculate the ID).
• if the attribute 'id' is modified, the state retention behaviour becomes non-deterministic in a similar way as before (when modifying the label).
• the value of 'id' must be unique within all elements of the tree. As a consequence if a TreeItem is re-parented (moved from one parent node to another node), its selection and expansion state is retained. In the old model the state was always lost in that case.

[sandy081]

Here is the strategy we concluded:

If TreeItem provides id, then this will be used as id which has to be unique for the complete tree. Otherwise, a unique id will be generated from its parent id and its label.