PDA

View Full Version : [FIXED-716] Node Cascade bug



Green
15 Jan 2008, 12:37 AM
I've reported this issue a long time ago in ExtJS 1.0. It has not been fixed as of yet. I did a quick search for it to see if it was reported for 2.0, but could not find anything specific.

The ExtJS docs is clear, "If the function returns false at any point, the cascade is stopped", which is simply not true. Way back when, I then resorted to override this behaviour within my own TreeNode class like so (which is also my proposed solution):


cascade : function(fn, scope, args){
if(fn.call(scope || this, args || this) !== false){
var cs = this.childNodes;
for(var i = 0, len = cs.length; i < len; i++) {
if (cs[i].cascade(fn, scope, args)==false) return false;
}
} else return false
},

which ensures the cascade stops the moment you return false in your function and stops the recursion dead, i.e. this cascade method gives you the behaviour ExtJS promises. The basic issue is that one can not implement a recursive function which returns nothing at all. The very basis of recursion is that the initial call is dependent on the return value of subsequent calls. As is, the default ExtJS cascade function will simply continue on with the siblings and their children even though you returned false at some point.

elDub
9 Jun 2008, 12:08 PM
Can this bug report get some love? I'm running into the same issue where cascade on an Ext.data.Node does not stop when I return false. According to the trunk of the repository, this hasn't been addressed.

wemakeitwork
5 Jan 2009, 9:17 AM
I do agree with Green. The current behavior (Ext 2.2) of TreeNode.cascade is not what I was expecting!

evant
5 Jan 2009, 5:04 PM
The documentation states:



If the function returns false at any point, the cascade is stopped on that branch.


Is the behaviour you're expecting the cascade should stop completely? Because that's not the intended behaviour. See this example:



Ext.onReady(function(){
var tree = new Ext.tree.TreePanel({
width: 800,
height: 400,
autoScroll: true,
renderTo: document.body,
buttons: [{
text: 'Cascade',
handler: function(){
tree.getRootNode().cascade(function(node){
console.log(node.id);
if (node.id == 'Parent2'){
return false;
}
});
}
}],
root: {
nodeType: 'node',
text: 'root'
}
});
var p = tree.getRootNode();
for (var i = 0; i < 5; ++i){
var n = p.appendChild(new Ext.tree.TreeNode({
text: 'Parent ' + i,
id: 'Parent' + i
}));

for (var j = 0; j < 5; ++j){
n.appendChild(new Ext.tree.TreeNode({
text: 'Child ' + j,
id: 'Child' + i.toString() + j.toString()
}));
}
}
});

Green
6 Jan 2009, 12:45 AM
Sorry to be a stickler Evant, but that is not what the docs said a year and a half ago, and changing the docs doesn't resolve the issue nor the requirement, it only clarifies the initial intentions.

a) Your example does not demonstrate any real world requirement I've come across. Why would I ever want to cascade down a subtree only to stop the cascading down some branches? This would surely only be needed in extremely rare cases. Why then should I have to wait for the cascade to finish if I already found what I was looking for in all the more popular cases?

b) As you can see there are many people looking for the expected functionality, what the initial intention was is therefore a moot point. Many real world examples do exist for the expected behavior, for ex. finding a specific node in a subtree based on some arbitrary condition (which is usually why people try to use cascade). As this latter requirement is more likely than any other, the current default code causes superfluous iterations, especially in huge trees.

c) It is easy to fix in the source and easier still to, as an alternative, supply a second function in the source which does actually work as expected. It is also not too hard to change cascade to be backward compatible while also allowing another boolean parameter to get the expected behavior.

Besides these points, the users demand it. Currently the code supplies 1 solution to 1 rare requirement at the expense of a 1,000 more popular requirements. Or in other words, currently the code is specific and not generic.

hendricd
6 Jan 2009, 9:03 PM
@Green -- It's likely that the intended design of cascade (and bubble) (as is the case with Ext.each) use the 'false return' as an means to terminate iteration. But that feature, coupled with Javascript's closures, achieve what your after (I think?), and more:


var pantLengths = [];
var noWaist;
var findNodeByPantLength = function(node){
var pantLength = node.attributes.inSeam;
if(pantLength == 0) {
noWaist = node;
return false; //stop iteration
}
pantLengths.push(pantLength);

}

node.cascade( findNodeByPantLength );
if(noWaist){
adjustDiet(noWaist.id);
}

Green
7 Jan 2009, 1:12 AM
While you make a good point hendric with your sample (i.e. finding a subset of nodes in a subtree, which was most likely the intended use of cascade), your iteration will still run through completely even if stopped on some branches (i.e. every sibling and children thereof will still be inspected even if you stopped iteration on on node's branch).

Fact is, there is no easy method to simply find a single subnode in a subtree and stop all iteration dead once found. Not a biggy if you have 10 nodes in a subtree, but a huge issue if you have 10,000 (10 subnodes with 10 children each, etc. 3 levels deep). As all 10,000 will still be inspected even if you found what you searched for at node 23.

My argument is therefore for 'cascade' to provide for, and be optimized for, the most popular case of finding a single node as fast as possible (preferably in addition to cascade's other uses as in your example). The sample code I supplied does exactly that, i.e. allows you to still cascade and collect a subset of nodes (as in your example), but it also allows you to stop the cascade completely for single node searches. Best of both worlds?

evant
7 Jan 2009, 1:54 AM
Instead of changing the cascade method, I would prefer changing findChild and findChildBy, since the intention of those is to return a single child. I think it would be better off to include a 3rd param on those methods, deep, which would default to false.

Thoughts?

Green
7 Jan 2009, 2:51 AM
Perfect Evant! Have my vote.

evant
7 Jan 2009, 2:25 PM
Just did this quickly, can you test it out in your app:



Ext.override(Ext.data.Node, {
findChild : function(attribute, value, deep){
var cs = this.childNodes, res, n;
for(var i = 0, len = cs.length; i < len; i++) {
n = cs[i];
if(n.attributes[attribute] == value){
return n;
}else if(deep){
res = n.findChild(attribute, value, deep);
if(res != null){
return res;
}
}
}
return null;;
},

findChildBy : function(fn, scope, deep){
var cs = this.childNodes, res, n;
for(var i = 0, len = cs.length; i < len; i++) {
n = cs[i];
if(fn.call(scope || n, n) === true){
return n;
}else if(deep){
res = n.findChildBy(fn, scope, deep);
if (res != null){
return res;
}
}
}
return null;
}
});

Green
7 Jan 2009, 9:17 PM
Works perfectly, thanks :)

mwhite
11 Mar 2010, 8:42 AM
I have been using the override with the additional versions of findChild and findChildBy. It satisfies my requirement perfectly! We have just upgraded to ExtJS 3.1.1, and it doesn't look like this change has been incorporated into the core implementation of Node. Are there any plans to add it?

mystix
11 Mar 2010, 10:32 AM
[ moved to 3.x Bugs from 2.x Bugs ]

evant
18 Mar 2010, 7:56 PM
This has been added to SVN, rev 6325.