dijit._Container refactoring

[Les]

I created the new queryChildren() function and used it (when possible) where the getChildren() function was used before. In many cases we don't need to have the widgets, but just the widget DOM nodes. I see a significant performance improvement.

Test:
Go here: ​http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Tree.html

Run:

dijit._Widget.prototype.queryChildren = function(){
   return dojo.query("> [widgetId]", this.containerNode);
};

dijit._Widget.prototype.addChildNew = function(/*Widget*/ widget, /*int?*/ insertIndex){
	// summary:
	//		Makes the given widget a child of this widget.
	// description:
	//		Inserts specified child widget's dom node as a child of this widget's
	//		container node, and possibly does other processing (such as layout).

	var refNode = this.containerNode;
	if(insertIndex && typeof insertIndex == "number"){
		var children = this.queryChildren();
		if(children && children.length >= insertIndex){
			refNode = children[insertIndex-1];
			insertIndex = "after";
		}
	}
	dojo.place(widget.domNode, refNode, insertIndex);

	// If I've been started but the child widget hasn't been started,
	// start it now.  Make sure to do this after widget has been
	// inserted into the DOM tree, so it can see that it's being controlled by me,
	// so it doesn't try to size itself.
	if(this._started && !widget._started){
		widget.startup();
	}
	return widget;		// Widget
};

dijit._Widget.prototype.removeChildNew = function(/*Widget or int*/ widget){
	// summary:
	//		Removes the passed widget instance from this widget but does
	//		not destroy it.  You can also pass in an integer indicating
	//		the index within the container to remove

	var node;
	if(typeof widget == "number" && widget > 0){
		node = this.queryChildren()[widget];
		widget = dijit.byNode(node);
	}
	else if(widget && widget.domNode){
		node = widget.domNode;
	}

	if(node){ node.parentNode.removeChild(node); } // detach but don't destroy
	
	return widget;		// Widget
};

dijit._Widget.prototype.addChild = function(/*Widget*/ widget, /*int?*/ insertIndex){
	// summary:
	//		Makes the given widget a child of this widget.
	// description:
	//		Inserts specified child widget's dom node as a child of this widget's
	//		container node, and possibly does other processing (such as layout).

	var refNode = this.containerNode;
	if(insertIndex && typeof insertIndex == "number"){
		var children = this.queryChildren();
		if(children && children.length >= insertIndex){
			refNode = children[insertIndex-1];
			insertIndex = "after";
		}
	}
	dojo.place(widget.domNode, refNode, insertIndex);

	// If I've been started but the child widget hasn't been started,
	// start it now.  Make sure to do this after widget has been
	// inserted into the DOM tree, so it can see that it's being controlled by me,
	// so it doesn't try to size itself.
	if(this._started && !widget._started){
		widget.startup();
	}
	return widget;		// Widget
};

dijit._Widget.prototype.getChildrenNew = function(){
	// summary:
	//		Returns array of children widgets.
	// description:
	//		Returns the widgets that are directly under this.containerNode.
	return this.queryChildren().map(dijit.byNode); // Widget[]
};

dijit._Widget.prototype.hasChildrenNew = function(){
	// summary:
	//		Returns true if widget has children, i.e. if this.containerNode contains something.
	return this.queryChildren().length > 0;	// Boolean
};

dijit._Widget.prototype.getIndexOfChildNew = function(/*Widget*/ child){
	// summary:
	//		Gets the index of the child in this container or -1 if not found
	return dojo.indexOf(this.queryChildren(), child.domNode);	// int
};

var parent = dijit.byId('dijit__TreeNode_0');
var child = dijit.byId('dijit__TreeNode_6');  // Europe, index 3

console.time('getChildren');
for(var i=0; i<1000; i++) {
   parent.getChildren();
}
console.timeEnd('getChildren');

console.time('queryChildren');
for(var i=0; i<1000; i++) {
   parent.queryChildren();
}
console.timeEnd('queryChildren');

dijit._Widget.prototype.getIndexOfChildNew = function(/*Widget*/ child){
// summary:
// Gets the index of the child in this container or -1 if not found
return dojo.indexOf(this.queryChildren(), child.domNode); // int
}

// getIndexOfChild test
console.time('getIndexOfChild');
for(var i=0; i<1000; i++) {
   parent.getIndexOfChild(child);
}
console.timeEnd('getIndexOfChild');

console.time('getIndexOfChildNew');
for(var i=0; i<1000; i++) {
   parent.getIndexOfChildNew(child);
}
console.timeEnd('getIndexOfChildNew');

// removeChild/addChild by index test
console.time('removeAdd');
for(var i=0; i<1000; i++) {
   parent.removeChild(3);
   parent.addChild(child, 3);
}
console.timeEnd('removeAdd');

console.time('removeAddNew');
for(var i=0; i<1000; i++) {
   var widget = parent.removeChildNew(3);
   parent.addChildNew(widget, 3);
}
console.timeEnd('removeAddNew');

console.time('getIndexOfChild');
for(var i=0; i<1000; i++) {
   parent.getIndexOfChild(child);
}
console.timeEnd('getIndexOfChild');

console.time('getIndexOfChildNew');
for(var i=0; i<1000; i++) {
   parent.getIndexOfChildNew(child);
}
console.timeEnd('getIndexOfChildNew');

console.log('getChildren: ' + (parent.getChildrenNew().length == parent.getChildren().length));

console.log('hasChildren: ' + (parent.hasChildrenNew() == parent.hasChildren()));

console.log('getIndexOfChild: ' + (parent.getIndexOfChildNew(child) == parent.getIndexOfChild(child)));

Results:

FF 3.5
getChildren --> queryChildren: 233ms --> 144ms
getIndexOfChild -- getIndexOfChildNew: 254ms --> 155ms
removeAdd --> removeAddNew: 1237ms --> 567ms
getIndexOfChild --> getIndexOfChildNew: 254ms --> 153ms

IE6
getChildren --> queryChildren: 1594ms --> 1000ms
getIndexOfChild -- getIndexOfChildNew: 1594ms --> 1063ms
removeAdd --> removeAddNew: 7578ms --> 3063ms
getIndexOfChild --> getIndexOfChildNew: 1594ms --> 1063ms

Chrome 4
getChildren --> queryChildren: 184ms --> 112ms
getIndexOfChild -- getIndexOfChildNew: 184ms --> 111ms
removeAdd --> removeAddNew: 876ms --> 390ms
getIndexOfChild --> getIndexOfChildNew: 180ms --> 112ms

[bill]

Interesting. Looks like it definitely will speed things up.

The problem is though that, for example, on AccordionContainer, getChildren() only returns half of the widgets (hiding the AccordionButton widgets), but addChild(), removeChild(), etc. still work even though AccordionContainer doesn't redefine those methods to deal w/that fact.

I think if every widget has a queryChildren() method that returned the DOM nodes of the official children, that we could do this change, but we can't make that change until 2.0. Do you see a different way?

Also, these methods should all be on _Container, right, not _Widget? getChildren() for _Widget is more general since it can recurse to any level until it finds a widget (whereas _Container is structured, everything is directly under this.containerNode).

[Les]

Also, these methods should all be on _Container, right, not _Widget? getChildren() for _Widget is more general since it can recurse to any level until it finds a widget (whereas _Container is structured, everything is directly under this.containerNode).

These methods should all be on _Container. I did it just for testing.

I'm not familiar with the AccordionContainer?, but all these changes should be generic. All modified methods should behave exactly the same as the existing methods. There will be no change at all in functionality of all container methods.

The point is that we don't need the expensive mapping from nodes to widgets that getChildren() does. We often need just the widget nodes, so the mapping step can be skipped.

You should be able to safely add these changes for 1.4

[bill]

Try it out, I think you'll see that it doesn't work. Try adding and removing panes from an AccordionContainer.

[Les]

Please help me. Write a small example that works using the existing Container methods, and I'll update it using the new methods that I modified.

Use this page if you can: ​http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/layout/test_AccordionContainer.html

The dijit._Container.queryChildren() method that I added can be private. It's just a helper, but I made it public in case someone wanted to use it. The container changes are really quite simple. Actually, they simplify the exisiting code. I'm confident they will work.

[bill]

We already have such examples, see dijit/tests/layout/AccordionContainer.html. It's an automated test (look in your console output), and you can also do operations manually.

Many widgets subclass _Container. _Container's addChild(), removeChild(), destroyRecursive() etc. methods are all setup to work based on the subclass's definition of getChildren().

[Les]

Many widgets subclass _Container. _Container's addChild(), removeChild(), destroyRecursive() etc. methods are all setup to work based on the subclass's definition of getChildren().

Right, and the way I set up the example, I overwrote those overwritten methods, but it works for the Tree. Anyway, I believe the original changes are correct and effective.

I can create a regular (formal) patch that would be easier to test. This is just a performance enhancement. I'm sure you are busy fixing bugs for 1.4, and this is something that we can return to for a future release.

[Les]

The problem is though that, for example, on AccordionContainer?, getChildren() only returns half of the widgets (hiding the AccordionButton? widgets), but addChild(), removeChild(), etc. still work even though AccordionContainer? doesn't redefine those methods to deal w/that fact.

Now I have doubts. I don't think it's possible to always use queryChildren() in place of getChildren(). It may not work when getChildren() is overwritten b/c getChildren() may return fewer widgets, i.e queryChildren() will return more elements in some cases.

Feel free to close this ticket. It's not valid.

[bill]

OK, closing in favor of #10087.

[bill]

seems like these aren't really enhancements from the users' point of view, labeling as tasks instead