Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#9361 closed enhancement (fixed)

[PATCH] [CLA] dijit.Tree - add multi-parented items support

Reported by: alle Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

Attached patch add support for having more than one nodes referring to same item.

Attachments (3)

tree.patch (3.8 KB) - added by alle 11 years ago.
tree_dnd.patch (7.9 KB) - added by alle 10 years ago.
Add multi-parented item dnd support allowing item's re-use.
tree_dnd_test.patch (882 bytes) - added by alle 10 years ago.
Fix test_Tree_DnD.html for new multi-parent dnd support.

Download all attachments as: .zip

Change History (32)

Changed 11 years ago by alle

Attachment: tree.patch added

comment:1 Changed 11 years ago by bill

Presumably a dup of #5535?

Anyway thanks for the patch, will take a look!

comment:2 in reply to:  1 Changed 11 years ago by alle

Sorry for dup ticket, I didn't find it before.
Let me know if something in the patch has to be fixed.

comment:3 Changed 11 years ago by bill

Thanks, I'll take a look. My question is whether you've tested DnD, i.e. moving one of the same-item nodes from one parent to another. Actually what we need is a robot test for DnD.

The reason DnD is complicated is that it implemented so indirectly... dragging and dropping a node just sends delete and add operations to the data store, which then turns around and notifies the Tree that a delete and insert happened (that's two separate notifications!), and the tree has to "move" the node.

comment:4 Changed 11 years ago by alle

Good guess pointing to not well tested DnD.

Second attached patch add it but it's a bit more complex because I need to know if dropping item or a matching one is already present in model and model api didn't support it.

This patch fix a potential bug with a model using a lazy loading store when trying to add an item already present in store on server but that's not yet referenced on client (so no placeholder in client store).

comment:5 Changed 11 years ago by bill

OK thanks, this looks good.

I don't understand the last paragraph above though. It shouldn't matter if the store is lazy-loading (whatever that means) or not, as all stores have the same API and contract.

comment:6 Changed 11 years ago by bill

Oh also, can you attach a test case? Or are you testing with test_Tree_DnD.html ?

comment:7 in reply to:  6 Changed 11 years ago by alle

Replying to bill:

Oh also, can you attach a test case? Or are you testing with test_Tree_DnD.html ?

I'm testing with test_Tree_DnD.html
I don't see anything more to test.

comment:8 in reply to:  5 Changed 11 years ago by alle

Replying to bill:

I don't understand the last paragraph above though. It shouldn't matter if the store is lazy-loading (whatever that means) or not, as all stores have the same API and contract.

A lazy-loading store is a store that loads individual items and data as late as possible.

If I try to add to tree an item with same id of an item already present in store, store can signal the problem or try to resolve it.
If store is a lazy-loading one conflicting item could not be on client so store has no way to see the problem. Only when trying to save changes server give error but it's too late for tree.

Creating a new item if it's not present in tree was not a fully correct check.

Anyway what I have written can be wrong but with new check it's not a problem anymore.

comment:9 Changed 11 years ago by bill

Owner: set to bill
Status: newassigned

Hmm, yes I guess test_Tree_DnD.html is a good enough test. I can follow all the code branches by dragging the same object repeatedly from the "list of items to be categorized" into the bottom right tree, and then also dragging from the left tree to the right tree (both of those trees have the same model.) I want to add something where it does a dump of the entire store contents, to make sure that they are what they are supposed to be.

test_Tree_Notification_API_Support.html might also be useful for testing the changes to Tree.js itself.

comment:10 Changed 11 years ago by bill

Alle - this patch is looking but I think there might be a small bug.

I'm testing test_Tree_Dnd.html as follows:

  1. Drag vegetables from left of tree, dropping under "fruits". There should now be two vegetables nodes, the original node vegetables1 (child of "foods"), and the new node vegetables2 (child of fruits).
  2. At this point, I think there's a bug that the left tree didn't update.
  3. Move vegetables2 (the new node) from child of foods to child of cereals.
  4. Move vegetables1 (the original node) from child of fruits to child of citrus.
  5. At this point we end up w/two vegetables nodes, one a child of fruits and one a child of citrus. We should have two nodes, one a child of citrus and the other a child of cereals. Apparently the drop operation moved the cereals --> vegetables node instead of the node that was being dragged (the fruits --> vegetables node).

Can you look at that? Other than that it's looking great.

comment:11 in reply to:  10 Changed 11 years ago by alle

I'm having some very busy days but after them I'll try to solve the problem.

comment:12 Changed 11 years ago by bill

Cool, thanks. I'm working on automated tests for Tree DnD.

comment:13 Changed 10 years ago by bill

See also #9339, asking for _itemNodeMap to be a public variable (or to have a public method to access it).

Changed 10 years ago by alle

Attachment: tree_dnd.patch added

Add multi-parented item dnd support allowing item's re-use.

Changed 10 years ago by alle

Attachment: tree_dnd_test.patch added

Fix test_Tree_DnD.html for new multi-parent dnd support.

comment:14 Changed 10 years ago by alle

bill - New attached patches fix bugs you pointed to.

comment:15 Changed 10 years ago by bill

Hmm, are you on IRC? I don't really understand why you need to modify the test? I on as wild_bill.

comment:16 Changed 10 years ago by alle

I'm not on IRC, sorry.
My not at all fluent english make IRC a bit problematic ;-)

I need to modify test because I have to add a method to model that say what children attribute use to add an external item to a parent.
Default this.childreAttrs[0] is not always a good choice and for test_Tree_DnD.html is wrong because categories have to be added to "children" attribute, not "items" attribute. So model has to redefine getChildrenAttrForItem method returning "children" for categories and "items" for items.

comment:17 Changed 10 years ago by bill

Ah, I see, it's a problem where you drop childCategory onto parentCategory and childCategory is added to parentCategory.items instead of parentCategory.children? I see the this.childrenAttrs[0] reference in TreeStoreModel.pasteItem. OK, makes sense, thanks!

comment:18 Changed 10 years ago by bill

I didn't mean to spend so much time dwelling on multiple child attribute support. I'm not sure we have a sound infrastructure for dealing with that for drag & drop anyway.

In your latest code you've added a new method to the model interface called getChildrenAttrForItem(), which is called by pasteItem(). Actually, it's just a helper for TreeStoreModel.pasteItem(), so I don't see a reason to add it to the model interface.

But, I think we can get rid of the method altogether if we can simply change this block of code in dndSource.html:

}else if(model.isItem(childItem)){
	// Item from same model
	model.pasteItem(childItem, null, newParentItem, true, insertIndex);

to:

}else if(model.isItem(childItem)){
	// Item from same model
	model.pasteItem(childItem, oldParentItem, newParentItem, true, insertIndex);

That way pasteItem() will detect that the dragged item is in oldParentItem's "children" attribute, and thus will add it to the "children" attribute of newParentItem.

Does that make sense?

Another question is whether that should be a move or a copy, see #7651. Of course users can always override pasteItem() to customize that behavior.

comment:19 Changed 10 years ago by bill

PS: I should point out that the entire concept of children attribute(s) is specific to TreeStoreModel. In other stores there may be (for example) a parent attribute instead.

comment:20 Changed 10 years ago by alle

I agree to change code as you suggested. This change move/copy too.
Now I see that code change don't solve children attribute issue because it's needed for newItem() method. I missed it in my patch but for items from external source it's needed. Maybe it can be a TreeStoreModel? only method.

Some comment on related #7651 issue later.

comment:21 Changed 10 years ago by bill

Right, for handling drops from external sources, when there are multiple child attributes, developers need to override newItem(), to decide which child attribute to use.

OK, I made that change to the pasteItem() call but I'm still seeing the issue I described above. The root problem seems to be in tree/_dndSelector.js which sets this.selection based on the item id, rather than the TreeNode id.

It's getting confused because the test I listed above switches between dragging two TreeNodes that both point to the same item (even though I'm never dragging both of those TreeNodes at once).

I'm looking into it.

comment:22 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [18027]) Support multi-parented items in Tree (by creating separate TreeNodes that point to the same item). Patch from alle (CLA on file), with some slight modifications from me. Thanks for the patch!

Automated test checkin (for the test listed in #9361) will be checked in shortly, and also a fix to #7651.

Fixes #9361 !strict.

comment:23 Changed 10 years ago by alle

In tree/_dndSelector.js line 78 is wrong because widget id is assigned to treeNode.rowNode (see tree/_dndContainer.js at line 77) so both rowNode and domNode have same id.

			var treeNode = dijit.getEnclosingWidget(this.current),
				id = treeNode.id;

			if (!this.current.id) {
				this.current.id=id;
			}
		onMouseOver: function(/*TreeNode*/ widget, /*Event*/ evt){
			// summary:
			//		Called when mouse is moved over a TreeNode
			// tags:
			//		protected
			this.current = widget.rowNode;
			this.currentWidget = widget;
		},

some kind of suffix is needed for rowNode id

			var treeNode = dijit.getEnclosingWidget(this.current),
				id = treeNode.id + "-rowNode";

			if (!this.current.id) {
				this.current.id=id;
			}

comment:24 Changed 10 years ago by bill

Good catch, will fix.

comment:25 Changed 10 years ago by bill

(In [18036]) Automated tests for multi-parented items DnD in Tree. Refs #9361.

comment:26 Changed 10 years ago by bill

(In [18037]) Drag and source and widget were getting the same id, thus having two DOMNodes w/the same id. Refs #9361.

comment:27 Changed 10 years ago by bill

(In [19638]) Update DnD API doc, in particular about the ({id: 123, data: "hello world", type: text?}) type objects that are passed to / returned from various methods. I've named this dojo.dnd.Item since it's returned by getItem(), although it shouldn't be confused with dojo.data.Item. (refs #9684).

Also updating Tree.model API doc for extra parameters added to newItem() for multi-parented items support (refs #9361).

!strict.

comment:28 Changed 10 years ago by bill

(In [19959]) Refactor code that checks if externally drag object represents an item that's already in the store. Fixes some issues such as the previously hardcoded name of the id attribute, and also allows writing custom models that don't do two XHR's for a drop (fetchItemByIdentity() followed by newItem()).

Refs #9361 !strict.

comment:29 Changed 10 years ago by bill

(In [19979]) Fix case problem, refs #9361.

Note: See TracTickets for help on using tickets.