Opened 8 years ago

Closed 8 years ago

#13713 closed defect (fixed)

Tree: problem making node a child of itself

Reported by: Siqi Owned by: bill
Priority: high Milestone: 1.7
Component: Dijit Version: 1.6.1
Keywords: dijit.Tree, DnD Cc: zhongsq@…
Blocked By: Blocking:

Description

Open dijit/tests/tree/test_Tree_DnD.html, following DnD operations on the 'collectionsTree' result in unexpected structure changes.

  1. Hold on the copy key ('ctrl' for windows), copy the node 'Cereals' and append it to 'Vegetables' through DnD operation.
  2. Release the copy key, try to move the original 'Cereals' to under the new 'Cereals'.

Result: The second DnD operation cannot be done correctly.

Attachments (1)

dnd.png (14.8 KB) - added by Siqi 8 years ago.

Download all attachments as: .zip

Change History (5)

Changed 8 years ago by Siqi

Attachment: dnd.png added

comment:1 Changed 8 years ago by bill

Summary: Internal DnD issue in dijit.Tree.Tree: problem making node a child of itself

Actually, the "copy" operation isn't really copying the Cereals item, but merely setting two parents for that item. Then when you try to make Cereals a child of Cereals that is making a node a child of itself, which isn't allowed. #7140 was supposed to explicitly disallow that but I guess it doesn't handle this case. I'll look into why.

comment:2 in reply to:  1 Changed 8 years ago by siqi

Replying to bill:

Actually, the "copy" operation isn't really copying the Cereals item, but merely setting two parents for that item. Then when you try to make Cereals a child of Cereals that is making a node a child of itself, which isn't allowed. #7140 was supposed to explicitly disallow that but I guess it doesn't handle this case. I'll look into why.

Seems I misunderstood the meaning of the 'copy' operation here. It sounds more like the 'link' semantic?

In the fix of #7140: following code in dndSource.js checks whether the node is in the selection by widget id rather than data item id. In the 'Cereals' case, although two 'Cereals' node share the same item, their widget id are different.

if(m.source == this && (newTarget.id in this.selection)
Last edited 8 years ago by siqi (previous) (diff)

comment:3 Changed 8 years ago by bill

Milestone: tbd1.7
Priority: highnormal
severity: majornormal

Thanks, you are right about the fix, I'll add that in.

As for copy vs. link, yes, the default is to link. I realize that in many cases, like windows explorer, "copy" makes more sense than "link", but in those cases apps just need to supply a custom pasteItem() implementation, one that generates a new item with a new id. Even if TreeStoreModel had code to generate a new item, it would still need help to get a new id.

Note that sometimes link makes more sense than copy, such as an organization tree for a company. You don't want to create a new copy of an employee, because it would imply that you had actually cloned that employee :-). But perhaps most of the time copy is wanted.

comment:4 Changed 8 years ago by bill

Owner: set to bill
Resolution: fixed
Status: newclosed

In [26413]:

Guard against making a node a child of a different node representing the same item. That would result in making an item a child of itself. Fixes #13713 !strict.

Note: See TracTickets for help on using tickets.