Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10071 closed enhancement (wontfix)

dijit._Container add firstChild() and lastChild() methods

Reported by: Les Owned by:
Priority: high Milestone: tbd
Component: Dijit Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

Looking at the dijit.Tree code I noticed that the Tree widget would benefit by being faster and easier to read if the dijit._Container had firstChild() and lastChild() methods.

Here are two examples:

// Current code
while(node.isExpanded){
    var c = node.getChildren();
    node = c[c.length - 1];
}

// New code
while(node.isExpanded){
    node = node.lastChild();
}
// Current code
var fc = this.tree.showRoot ? this : this.getChildren()[0];

// New code
var fc = this.tree.showRoot ? this : this.firstChild();

Change History (5)

comment:1 Changed 10 years ago by bill

That's would make tree easier to read, although it would increase the code size of _Container merely for the benefit of one widget. (Thus increasing the download size for anyone using _Container but not using Tree).

Also, I don't see how it would make things faster. Probably you want to use dojo.query() to get the first and last child? But try it on AccordionContainer and you'll see that it doesn't work, specifically that first() will return the hidden AccordionButton as the first child, rather than the first pane like it should.

comment:2 Changed 10 years ago by Les

Why dijit._Container methods use dojo.query to get child widgets? This is not efficient.

dijit._Container provides a complete API for adding and removing widgets. These methods should maintain an internal widget cache - childWidgets in the example below. This design would be much simpler and faster.

// Current code
getChildren: function(){
  return dojo.query("> [widgetId]", this.containerNode).map(dijit.byNode); // Widget[]
}

// New code
getChildren: function(){
  return this.childWidgets; // Widget[]
}

// New code
hasChildren: function(){
  return this.childWidgets.length > 0;	// Boolean
}

comment:3 Changed 10 years ago by bill

Resolution: wontfix
Status: newclosed

That might be a good idea, although I've just spent the past few weeks incorporating suggestions from use to use dojo.query() more, for example [20296].

It's unclear though why you want to change the current code though, as I haven't seen any evidence that there's a real performance issue. (I.e.: There's surely a benefit when calling getChildren() thousands of times in a test loop, but is there an issue when loading a page or using the widgets? That's what you need to show before we complicate the code to add a cache.)

Anyway, if you want to open a new ticket and write a complete patch to do this I'll be happy to take a look.

I'm going to close this ticket since I don't want to increase the size of dijit._Container just to benefit Tree.

comment:4 in reply to:  3 Changed 10 years ago by Les

Replying to bill:

It's unclear though why you want to change the current code though

I'll give you an example. Let's say you have a Tree node with 1,000 children. If you want to get the first or last child of the Tree node, you will first need to call dojo.query to return 1,000 DOM nodes, then call dijit.byNode 1,000 times to get all child widgets all this to get just one widget. Sure, this may amount to 50 ms (vs 0 ms if the code was modified not to rely on dojo.query), but it just does not make sense to build a NodeList? of 1,000 widgets to get just one widget.

I'll create a patch, no problem. You will see that the code is a lot simpler and faster w/o relying on dojo.query. I think the code (with the added functionality) will show little increase in size.

comment:5 in reply to:  3 Changed 10 years ago by Les

Replying to bill:

.. I've just spent the past few weeks incorporating suggestions from use to use dojo.query() more, for example [20296].

BTW I feel the same way. I spent hours understanding the original code and how to improve it, and it just struck me that the getChildren() logic to query the DOM and then mapping the nodes to widgets is completely unnecessary. Anyway, a fully tested patch is forthcoming.

Note: See TracTickets for help on using tickets.