PDA

View Full Version : [FIXED-540][3.1.1-rc] TreePanel DnD reordering places dropped item at bottom of tree



stevestorey
7 Feb 2010, 10:05 AM
Ext version tested:


Ext 3.1.1-rc rev 5977 (public RC)



Adapter used:


ext


css used:


only default ext-all.css



Browser versions tested against:



FF3.6 (firebug 1.5.0 installed)



Operating System:


Linux



Description:


Test Case:

Compare this URL : http://www.extjs.com/deploy/ext-3.1.1-rc/examples/tree/reorder.html
(http://www.extjs.com/deploy/ext-3.1.1-rc/examples/tree/reorder.html) with:
http://www.extjs.com/deploy/dev/examples/tree/reorder.html


Steps to reproduce the problem:


Open 3.1.1-rc sample page
Drag any item within the list
Drop it
See that dragged item now appears at bottom of tree



The result that was expected:


The dragged item remains where it was dropped (as happens on the /dev/ code)



The result that occurs instead:


The dragged item now appears at bottom of tree, not where it was dropped.



Screenshot or Video:


N/A



Debugging already done:


None - however, I have determined that the movenode event fires with the correct index where the item was dragged to.



Possible fix:


not provided

aconran
7 Feb 2010, 12:09 PM
I can confirm this behavior in 3.1.1 RC.

hhangus
8 Feb 2010, 12:41 PM
Seeing this in FF3.6 on Windows, and IE8. I tried tracking it down and got as far as identifying the function which is causing the issue, though I wasn't sure what to do about it since the function did not exist in v3.0.3 and doesn't appear to have any actual purpose other than to frustrate users. I simply commented it out and everything works just fine for me.

In ext-all-debug.js, in the


Ext.extend(Ext.tree.TreeNode, Ext.data.Node, {
...
// private override
insertBefore : function(node, refNode){
var newNode, exists;
if(!node.render){
node = this.getLoader().createNode(node);
} else {
exists = Ext.isObject(node.parentNode);
}
newNode = Ext.tree.TreeNode.superclass.insertBefore.call(this, node, refNode);
if(newNode && refNode){
this.afterAdd(newNode, exists);
}
this.ui.updateExpandIcon();
return newNode;
},

// private
afterAdd : function(node, exists){
if(this.childrenRendered){
// bulk render if the node already exists
node.render(exists);
}else if(exists){
// make sure we update the indent
node.renderIndent(true, true);
}
},
...
}

stevestorey
8 Feb 2010, 1:23 PM
Simply commenting out one of the calls didn't work for my particular use case, as I'm also dynamically adding nodes from a toolbar, which failed when commenting out the calls.

However - while I don't fully understand this code, I'm assuming that the afterAdd call should only be called if it was genuinely added rather than a pre-existing node being inserted, and so the following change in insertBefore does the trick for me - HOWEVER! I have no idea why ;) - YMMV!



if(newNode != node && refNode){
this.afterAdd(newNode, exists);
}


EDIT! I take it all back - I was commenting out the call within the afterAdd, rather than the call to afterAdd itself. After commenting that out, I agree it all works.

hhangus
8 Feb 2010, 2:09 PM
Your solution (newNode!=node) works for me too, and also fixes some issues I was having after removing the function entirely. I still have no idea what its doing, but hopefully this is a real fix and can be included in the next RC.

stevestorey
8 Feb 2010, 3:16 PM
Having looked a bit more at the code, I don't think it's that simple unfortunately, but the if statement is clearly wrong.

The Ext.tree.TreeNode.superclass.insertBefore call returns either the node that was added (i.e. newNode == node) or false (if the node wasn't added, e.g. because a "beforeXXX" event said "no").

If the call returns false, then afterAdd shouldn't be called, but equally, I guess that the afterAdd should only be called if the node wasn't part of the tree before? Then again maybe not, since it would place the new node at the bottom of the tree as well rather than where it was added? Or is the true bug in the render code, which is mistakenly forcing the node to render at the bottom of the tree?

That whole logic to me doesn't make sense.

Both our fixes should presumably cause problems if you're calling TreeNode#insertBefore with a Node that wasn't previously in the tree (yours will make it never get called - mine will call afterAdd if the node wasn't added, which can't be right ...). For my specific use-case, either is fine (and yours is more obvious) as I'm never inserting brand new nodes, but explictly appending them to the bottom, which does work.

hhangus
9 Feb 2010, 4:33 PM
FYI, I'm sorry if none of this makes sense to you, if you get lost, my quick-fix is at the end lol.

After further testing I discovered your change breaks node.replaceChild(newNode, oldNode). I traced through what was happening and the following methods are called in this order:


Ext.data.Node.replaceChild(...)
Ext.tree.TreeNode.insertBefore(...)
Ext.tree.TreeNode.afterAdd(...)
Ext.tree.TreeNode.render(bulkRender)

The key here is 'bulkRender' which is the value of (bool)'exists' passed from afterAdd(). In other words, if the node exists in the tree then the function called is node.render(true). Looking in that function we see that 'bulkRender' causes this function to (re-)append the passed node to it's parent, rather than simply rendering it in place.

So the whole issue appears to boil down to 'bulkRender' being used improperly. At least, that's what I assume, since I'm still not sure what it's proper use actually is.

I think the devs need to backtrace what that whole afterAdd() function was meant to do since it clearly breaks more than it fixes.

Lastly, the following simple modification has fixed everything (so far) for me:


// private
afterAdd : function(node, exists){
if(this.childrenRendered){
// bulk render if the node already exists
node.render(); //notice there is no exists passed to render.
}else if(exists){
// make sure we update the indent
node.renderIndent(true, true);
}
},

anakonda8472
10 Feb 2010, 4:39 AM
Hello!

Yes, this quick fix also solved all my problems with tree reordering.

I put the following code in a separate file executed
after the "ext-all.js" file:


Ext.override(Ext.tree.TreeNode, {
afterAdd : function(node, exists){
if (this.childrenRendered) {
// bulk render if the node already exists
node.render(); //notice there is no exists passed to render.
} else if(exists){
// make sure we update the indent
node.renderIndent(true, true);
}
}
});


Thanx!

cstansbury
12 Feb 2010, 2:36 PM
I've replicated the bug and the patch *mostly* works, but not in the situation where I try to make the last child the first child (e.g., parent.insertBefore(child[last],child[1])). It results in the last child being moved to index = 1 rather than index = 0.

Ignore this - it was a bug on my part. The patch *does* appear work.

evant
18 Feb 2010, 4:53 AM
I've reverted the change that caused this. Should be fixed in SVN.

gnaegi
4 Mar 2010, 2:51 AM
I've reverted the change that caused this. Should be fixed in SVN.

This URL is still broken:
http://www.extjs.com/deploy/dev/examples/tree/reorder.html

Isn't this the SVN version?

Jamie Avins
4 Mar 2010, 9:30 AM
No, that is the latest public release (3.1.1).

mrusinak
11 Jun 2010, 2:21 PM
Hello,

I am using 3.2.1, and was basically running into the same problem but manifested in a different way. I am using the TreeGrid extension, with a 2-level tree in which each level should stay at its own level. I was listening to the 'nodedragover' event to disallow drops on the wrong types (leaf onto leaf, leaf onto root, etc), which was simple enough. However, after stopping leaf-onto-root (which is always an Append), most of the time when dragging a leaf over a leaf I would get the "Drop not allowed" icon.

Using Firebug, I found that the event was coming through onContainerOver... which wasn't right. I tracked it down to a problem with TreeDropZone incorrectly finding the row the event target belongs to.

Here is my override that fixes it:
Ext.override(Ext.tree.TreeDropZone, {
getTargetFromEvent: function(e) {
var t = e.getTarget('tr.x-tree-node-el', 6);
return (t) ? Ext.dd.Registry.getTarget(t) : t;
}
});