Use tree item path for tree node id. Fixes #12111 #12120

tsmaeder · 2023-01-27T13:44:52Z

Since our front end tree is not safe for reparenting nodes, use the path of tree item ids as the tree node id.
Rename TreeViewExt.getTreeItem to getElement, since that is what it does.

Contributed on behalf of ST Microelectronics

Signed-off-by: Thomas Mäder t.s.maeder@gmail.com

What it does

Fixes #12111
Since the Theia front end tree cannot handle reparenting tree nodes with the same id, this PR fixes the problem for tree views contributed via the VS Code API. The idea is to concatenate the id of the parent TreeItem with the id of the tree item itself to use as the id of the TreeViewItem that is sent to the front end. Thus, the id of the tree node in the front end will be different when being reparented, thus working around the problem.

How to test

Install the extension from #12065 (comment) and test that elements can be properly reparented and that selection and expansion behaviour is as expected. Note that, since the id changes when reparenting, an item that is dragged to a different folder will not be selected after the move. I consider this an acceptable trade-off considering the current state may lead to an endless loop. I was not able to think of a fix that would avoid this without thoroughly refactoring the tree subsystem.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

paul-marechal · 2023-02-06T20:51:07Z

packages/plugin-ext/src/plugin/tree/tree-views.ts

+        const candidateId = this.buildTreeItemId(parentId, treeItem);
+        if (this.nodes.has(candidateId)) {
+            return chain.concat(candidateId);


Does this new code mean we'll never hit the cache for tree items for which we compute an ID?

The previous logic seemed to assume we could find items from some possibleIndex, but now there's a case where buildTreeItemId generates a new unique ID for each call, which means that those items will never be found here.

Is this an issue?

Only if the tree item does not have an id set. This is correct, in my opinion, because if it doesn't have an id, there's no way to tell it's the same tree item, so any state we might have for the tree item at index n might not apply to the new item at index n.

Also, the documentation of TreeItem says:

If not provided, an id is generated using the tree item's label. Note that when labels change, ids will change and that selection and expansion state cannot be kept stable anymore.

If you look at the way id's are constructed, the id's will be the same for items that have the same label.

colin-grant-work · 2023-02-07T20:27:25Z

Since the Theia front end tree cannot handle reparenting tree nodes with the same id...

This seems unfortunate. Can you give an idea of the code to reparent a node that you think should work (in an ideal world), but doesn't?

tsmaeder · 2023-02-08T10:12:40Z

Since the Theia front end tree cannot handle reparenting tree nodes with the same id...

This seems unfortunate. Can you give an idea of the code to reparent a node that you think should work (in an ideal world), but doesn't?

Any time you use the same id with a different parent, think dragging a file to a different folder (the "real" file explorer uses the full path as a key, which is what I'm doing in this PR). The problem is described in #12111

tsmaeder · 2023-02-21T13:29:00Z

@paul-marechal @marcdumais ping?

tsmaeder · 2023-02-21T15:11:51Z

Can you give an idea of the code to reparent a node that you think should work (in an ideal world), but doesn't?

@colin-grant-work the code assumes that there is a 1:1 relationship from tree item to underlying domain element. Since we're doing an asynchronous update for the tree, this is not true when reparenting a domain object (because some of the tree is still using the "old" state, while other parts are using the "new" state). So having code that can handle n:1 between tree items and domain objects would work. It would also allow showing DAGs as a tree.

colin-grant-work

Functionally, things appear to be working. The only missing element there, not related to this PR, is that we get no drag-over highlight on tree nodes when dragging over them.

I have some other commends and questions about the code.

packages/plugin-ext/src/plugin/tree/tree-views.ts

colin-grant-work · 2023-02-23T17:21:27Z

packages/plugin-ext/src/plugin/tree/tree-views.ts

@@ -274,7 +277,7 @@ class TreeViewExtImpl<T> implements Disposable {
        this.proxy.$setDescription(this.treeViewId, this._description);
    }

-    getTreeItem(treeItemId: string): T | undefined {
+    getElement(treeItemId: string): T | undefined {


Same here re: naming.

And same answer ;-)

packages/plugin-ext/src/plugin/tree/tree-views.ts

colin-grant-work

Allows nodes to be moved among parents smoothly 👍

- Since our front end tree is not safe for reparenting nodes, use the path of tree item ids as the tree node id. - Rename TreeViewExt.getTreeItem to getElement, since that is what it does. Contributed on behalf of ST Microelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

Use tree item path for tree node id. Fixes #12111 - Since our front end tree is not safe for reparenting nodes, use the path of tree item ids as the tree node id. - Rename TreeViewExt.getTreeItem to getElement, since that is what it does. Contributed on behalf of ST Microelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

colin-grant-work · 2023-03-22T17:03:02Z

packages/plugin-ext/src/plugin/tree/tree-views.ts

+    private buildTreeItemId(parentId: string, item: TreeItem): string {
+        // build tree id according to https://code.visualstudio.com/api/references/vscode-api#TreeItem
+        // note: the front end tree implementation cannot handle reparenting items, hence the id is set to the "path" of individual ids
+        let id = typeof item.id === 'string' ? item.id : this.getItemLabel(item);
+        if (id) {
+            // we use '' as the id of the root, we don't consider that a valid id
+            // since '' is falsy, we'll never get '' in this branch
+            id = TreeViewExtImpl.ID_ITEM + id;
+        } else {
+            id = TreeViewExtImpl.ID_COMPUTED + this.nextItemId++;
+        }
+        return `${parentId}/${id}`;


@tsmaeder, this turns out to be a dangerous way to build the ID. If getItemLabel returns the same thing for several children of the same item - we've seen this downstream in the references view when the lines where something is referred to are identical - then this produces children with the exact same ID's, which isn't permitted in our core Tree implementation. Should be able to reproduce in Theia in a moment.

tsmaeder requested review from paul-marechal and marcdumais-work January 27, 2023 13:45

paul-marechal reviewed Feb 6, 2023

View reviewed changes

tsmaeder mentioned this pull request Feb 23, 2023

Backport fixes for 1.34.2 #12211

Closed

colin-grant-work reviewed Feb 23, 2023

View reviewed changes

colin-grant-work approved these changes Feb 24, 2023

View reviewed changes

tsmaeder and others added 6 commits February 27, 2023 09:55

Make the linter happy

326a643

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

remove unnecessary loop

58c3218

add helper comment

dc9a715

Add breaking change log

7dcc283

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

Undo method rename

b07dec7

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

tsmaeder force-pushed the 12111_reparent_safe_ids branch from 6acbbdf to b07dec7 Compare February 27, 2023 09:01

tsmaeder merged commit 9952221 into eclipse-theia:master Feb 27, 2023

colin-grant-work reviewed Mar 22, 2023

View reviewed changes

vince-fugnitto added the tree issues related to the tree (ex: tree widget) label Mar 30, 2023

vince-fugnitto added this to the 1.36.0 milestone Mar 30, 2023

vince-fugnitto added the plug-in system issues related to the plug-in system label Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tree item path for tree node id. Fixes #12111 #12120

Use tree item path for tree node id. Fixes #12111 #12120

tsmaeder commented Jan 27, 2023 •

edited by colin-grant-work

Loading

paul-marechal Feb 6, 2023 •

edited

Loading

tsmaeder Feb 6, 2023

tsmaeder Feb 24, 2023

colin-grant-work commented Feb 7, 2023

tsmaeder commented Feb 8, 2023 •

edited

Loading

tsmaeder commented Feb 21, 2023

tsmaeder commented Feb 21, 2023

colin-grant-work left a comment

colin-grant-work Feb 23, 2023

tsmaeder Feb 24, 2023

colin-grant-work left a comment

colin-grant-work Mar 22, 2023

Use tree item path for tree node id. Fixes #12111 #12120

Use tree item path for tree node id. Fixes #12111 #12120

Conversation

tsmaeder commented Jan 27, 2023 • edited by colin-grant-work Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

tsmaeder Feb 6, 2023

Choose a reason for hiding this comment

tsmaeder Feb 24, 2023

Choose a reason for hiding this comment

colin-grant-work commented Feb 7, 2023

tsmaeder commented Feb 8, 2023 • edited Loading

tsmaeder commented Feb 21, 2023

tsmaeder commented Feb 21, 2023

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Feb 23, 2023

Choose a reason for hiding this comment

tsmaeder Feb 24, 2023

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Mar 22, 2023

Choose a reason for hiding this comment

tsmaeder commented Jan 27, 2023 •

edited by colin-grant-work

Loading

paul-marechal Feb 6, 2023 •

edited

Loading

tsmaeder commented Feb 8, 2023 •

edited

Loading