PDA

View Full Version : TreeNode: Multiple async calls creates duplicated children



jbraband
19 Jan 2007, 2:49 PM
all, I ran into a little bug that will probably rarely show itself, but i fixed it none-the-less.

Scenario using the .40 code from svn:

I had my debug process running on the script that TreeLoader calls to retrieve its data. this resulted in the UI spinning the little loading icon. I didnt realized that i had hit a breakpoint in visual studio and double clicked on the node multiple times while it was loading. Once i realized that i was at a breakpoint in the code, i continued through and to my surprise was presented with three copies of the children that were loaded were attached to the node.

To fix this TreeLoader needs to remove existing children of the node before attaching the fresh results from the async call. I chose to write a removeAllChildren function to simplify things a bit. Code for various classes is presented below. What are other's thoughts?

-------------------------------------------------------------------

modified YAHOO.ext.tree.TreeLoader.processResponse


processResponse : function(response, node, callback){
var json = response.responseText;
try {
var o = eval('('+json+')');
node.removeAllChildren();
for(var i = 0, len = o.length; i < len; i++){
node.appendChild(this.createNode(o[i]));
}
if(typeof callback == 'function'){
callback();
}
}catch(e){
this.handleFailure(response);
}
}



added function to YAHOO.ext.tree.TreeNode


removeAllChildren : function(){
var cs = this.childNodes;
for(var i = 0, len = cs.length; i < len; i++){
// unselect nodes
this.ownerTree.getSelectionModel().unselect(cs[i]);

if(this.childrenRendered){
// if it's been rendered remove dom node
node.ui.remove();
}
}

YAHOO.ext.tree.TreeNode.superclass.removeAllChildren.apply(this, arguments);

this.collapse(false, false);
return;
}



added function to YAHOO.ext.data.Node


removeAllChildren : function(){
if(this.fireEvent('beforeremove', this.ownerTree, this, null) === false){
return false;
}

this.childNodes = [];

this.fireEvent('remove', this.ownerTree, this, null);
return;
}



is this something that can be added to core files in svn? i've never been a part of an "open source" community and am not sure if that is something that I contribute to svn directly or if I prompt the need for the fix here and have jack review it and make the change.

-j

jack.slocum
20 Jan 2007, 5:37 AM
For removeAll, it's the same as standard DOM:


while(node.firstChild){
node.removeChild(node.firstChild);
}

The associated events will do the rest (unselect, remove html, etc).

Blanking the node on load will have to be an option, because people who have created the tree from existing markup or manually created nodes may have nodes there they want to keep. I've added a clearOnLoad option that defaults to true.

The actual code modification was a little shorter, this went on the beginning of the load method:


load : function(node, callback){
if(this.clearOnLoad){
while(node.firstChild){
node.removeChild(node.firstChild);
}
}
....

I would like to note that loading multiple times in a normal environment (not in a debugger) should not be possible.

jbraband
20 Jan 2007, 8:25 AM
regarding the possibilty of this happening...agreed.
couldnt it theoretically happen with an async call that is intensive (lots of data processing). granted...how intensive can processing the child nodes of a menu item be?

thanks jack for hearing the pee-ons :)
-j

jack.slocum
20 Jan 2007, 9:26 AM
As long as you aren't somehow doing the load call outside of the tree, it shouldn't happen. If you look at AsyncTreeNode, it queues the callback for multiple calls and only allows loading once.

jbraband
20 Jan 2007, 3:36 PM
simply genius

jack.slocum
21 Jan 2007, 6:55 AM
Ah actually this made me open the code and look and boom! The queueing block is missing one thing very important, a return statement at the end. It should be right after the call to setInterval.


...
timer = setInterval(f, 200);
return;

Without that return, the call gets queue'd but the normal expand code still gets executed! :(

I'm glad this thread continued because that would have been a hard one to figure out!

jbraband
21 Jan 2007, 11:57 AM
so with this, it makes the clearOnLoad config option completely obsolete, right?

jack.slocum
21 Jan 2007, 5:57 PM
Maybe, still makes sense to have it in there though I guess.