Opened 9 years ago

Closed 9 years ago

#11877 closed defect (fixed)

Tree: can't drop external items before/after existing Tree items

Reported by: Eric Pasquier Owned by: bill
Priority: high Milestone: 1.6
Component: Dijit Version: 1.5
Keywords: Tree TreeStoreModel Cc:
Blocked By: Blocking:

Description (last modified by bill)

The newItem function of TreeStoreModel is trying to pass an insertIndex parameter to store.newItem, which do not follow the dojo.data.api.write specification, and is not used by the store. This is making all new items created at the end of the children list.

The Model should managed the insert position after the creation of the new item.

For example, pasteItem can be used to move the position of the new Item :

// Create new item in the tree, based on the drag source.
LnewItem=this.store.newItem(args, pInfo);

// Move the new item to the insertIndex position
if (LnewItem && (insertIndex!=undefined)){
  this.pasteItem(LnewItem, parent, parent, false, insertIndex);
}

Attachments (6)

TreeStoreModel.js (12.9 KB) - added by Eric Pasquier 9 years ago.
TreeStoreModel.js.patch (1.4 KB) - added by Eric Pasquier 9 years ago.
test_Tree_DnD.html (7.9 KB) - added by Eric Pasquier 9 years ago.
test_Tree_DnD.html.patch (2.5 KB) - added by Eric Pasquier 9 years ago.
Tree_dnd.html (29.5 KB) - added by Eric Pasquier 9 years ago.
Tree_dnd.html.patch (10.8 KB) - added by Eric Pasquier 9 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by bill

Description: modified (diff)

I agree, looks like a bug. Are you seeing this problem when dropping external items onto the Tree in a specified position (where that horiziontal bar shows up to indicate drop position)?

comment:2 Changed 9 years ago by Eric Pasquier

The problem can be reproduced at any position of the horizontal bar, as the index position is not used by the store for creating the new item. It works if you call 'pasteItem' after the creation of the new item (twice in the code). I wanted to help by submitting a patch, but I didn't find how to generate it.

comment:3 Changed 9 years ago by bill

You can probably generate a patch from your IDE, or TortoiseSVN using a "create patch" menu option. Not sure what your environment is. Can also google for "svn unified diff".

Before doing that though, you need to file a CLA if you haven't, see http://dojofoundation.org/cla/.

The biggest thing though is that we need to add automated tests for any new features or bug fixes, see dijit/tests/tree/robot/Tree_DnD.html for the existing test.

Thanks!

comment:4 Changed 9 years ago by Eric Pasquier

I attached the following files :

  • dijit/tree/TreeStoreModel.js: added call to pasteItem after the creation of the newItem (twice)
  • dijit/tests/tree/test_Tree_Dnd.html: added betweenThreshold parameter
  • dijit/tests/robot/Tree_dnd.html: added 6 tests, drag before/after on first item, middle of the last and last item.

You'll please excuse me as I wasn't able to give you a patch file. I already signed the CLA so I hope this will help to make the change for the next release.

comment:5 Changed 9 years ago by bill

Thanks for the changes, but please try to use patch. The main problem I'm seeing is that you changed old versions of the files, maybe from 1.5? It's too hard for me to figure out what you changed and integrate it into the latest code.

comment:6 Changed 9 years ago by Eric Pasquier

Yes I changed it from the 1.5 release, as I thought it was the last one. Can you please tell me which path should I use for the initial checkout, like http://svn.dojotoolkit.org/src/tags/release-1.5.0/dijit/. I'll try to use TortoiseSVN (on Windows) to generate the patch file.

Changed 9 years ago by Eric Pasquier

Attachment: TreeStoreModel.js added

Changed 9 years ago by Eric Pasquier

Attachment: TreeStoreModel.js.patch added

Changed 9 years ago by Eric Pasquier

Attachment: test_Tree_DnD.html added

Changed 9 years ago by Eric Pasquier

Attachment: test_Tree_DnD.html.patch added

Changed 9 years ago by Eric Pasquier

Attachment: Tree_dnd.html added

Changed 9 years ago by Eric Pasquier

Attachment: Tree_dnd.html.patch added

comment:8 Changed 9 years ago by Eric Pasquier

Please find the patch files and also updated plain text files.

comment:9 Changed 9 years ago by bill

Milestone: tbd1.6
Owner: set to bill
Status: newassigned

Thanks! BTW for next time it's easier to put everything into one big patch file, and there's no need to attach the original files. Also, our coding standards say to use tabs rather than spaces.

Anyway, the patch looks good, and I'll check it in. The one thing I'm unhappy with is that we have to do two operations on the store, but as you said that's a limitation of store.newItem().

Although, perhaps it's better to call store.newItem() without a parent specification, and then adjust the parent's children afterwards. In any case this will change a bit in 2.0 when we switch to using the new dojo.store.

comment:10 Changed 9 years ago by bill

Summary: TreeStoreModel: incorrect insertion of new itemsTree: can't drop external items before/after existing Tree items

comment:11 Changed 9 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [23314]) Fix dropping of external items in-between existing Tree items. Fixes #11877 !strict. Patch from Eric P, thanks!

Note: See TracTickets for help on using tickets.