Opened 10 years ago

Closed 9 years ago

#10087 closed task (fixed)

dijit_Container refactoring to eliminate dojo.query

Reported by: Les Owned by: bill
Priority: high Milestone: 1.5
Component: Dijit Version: 1.3.2
Keywords: Cc: bill, Adam Peller
Blocked By: Blocking:

Description

Bill, I attached the refactored diji._Container mixin. I removed the need to query or traverse the DOM to get child info. The code is now much simpler and faster since it doesn’t rely on the intermediate dojo.query step.

You will see that the container methods maintain the in-memory ‘children’ NodeList? – all container objects now have this object.

I tested the changes extensively using the Tree widget. I didn’t test it using other widgets that contain fake children, but I believe the code should be also be solid for these classes.

I’d appreciate if you could take a look at the code to see the idea.

Attachments (1)

_Container.js (4.8 KB) - added by Les 10 years ago.

Download all attachments as: .zip

Change History (13)

Changed 10 years ago by Les

Attachment: _Container.js added

comment:1 Changed 10 years ago by bill

OK thanks, please try running the regression tests for dijit with the new changes (even though you believe the code is solid).

The other thing is that, can you attach a patch file instead of a whole _Container.js? That's the way that we take patches to the code, so it's clear what part you are changing.

BTW about these lines:

if(index){
        this.children.splice(index, 0, widget);
}else{
        this.children.push(widget);
}

It looks wrong because the if() won't fire when index is 0.

Actually, can you add tests for addChild() into _Container.html? Thanks.

comment:2 Changed 10 years ago by Adam Peller

Cc: Adam Peller added

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

Replying to bill:

OK thanks, please try running the regression tests for dijit with the new changes (even though you believe the code is solid).

How do I do this? Do I have to install a server? Is it possible to run the dijit regression test w/o a server?

comment:4 Changed 10 years ago by bill

Hmm, well the tests are run from dijit/tests/runTests.html, and an individual test like _Container.html can just be run by loading that file (look in the console to see the test results).

Many of the tests will run without a server although some need a server with PHP enabled.

comment:5 Changed 10 years ago by bill

Milestone: tbd1.5
Owner: set to bill

comment:6 Changed 10 years ago by Les

I see a little bit of a problem with the dojo.parser. It's not calling the addChild method on the container, so the list of children is not accumulating and the getChildren() method returns and empty list.

The parser would have to call the addChild method or insert the newly created instance into the children NodeList? of the parent.

comment:7 Changed 10 years ago by bill

That's a long term goal to think about, see http://thread.gmane.org/gmane.comp.web.dojo.devel/9682, but in the meantime could easily do an initial query to get all the children, maybe in startup() or maybe the first time that we need to reference the children array.

I was initially worried about slowing down initial page load performance by querying all the children but I guess that we need to do the query anyway because we do startup() calls on each child.

PS: I hope startup() is getting called on all _Container widgets. I know the parser does it, but do all developers do it for all programatically created widgets?

comment:8 Changed 10 years ago by Les

dijit._Contained code could be simplified if each child contained parent widget reference. Then, each container would have a list of children and each child would contain reference to its container.

Here's new dijit._Contained code

getParent: function(){
  return this.parent;
},

// Can be removed - not needed
_getSibling: function(/*String*/ which){
...
},

getPreviousSibling: function(){
  return this.parent._getSiblingOfChild(this, -1);
},

getNextSibling: function(){
  return this.parent._getSiblingOfChild(this, 1);
},

getIndexInParent: function(){
  return this.parent.getIndexOfChild(this); // int
}

comment:9 Changed 10 years ago by bill

Well, I think you could just as easily write those methods using this.getParent() rather than this.parent and the methods would be equally as simple, albeit not as fast. The issue again (with keeping this.parent cached) is keeping it up to date both on initial page load and when a child is added/removed from a parent node.

But anyway, if you want to supply a patch for that please open as a separate ticket (but I'd prefer finishing this ticket first, the _Container refactor).

comment:10 Changed 10 years ago by bill

Type: enhancementtask

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

comment:11 Changed 10 years ago by bill

One more comment:

Currently when there's a parent/child connection between widgets, calling childWidget.destroy() (or destroyRecursive()) sort-of works... meaning that parentWidget.getChildren() will no longer return that child. It doesn't completely work because (for example for TabContainer), IIRC the TabContainer's list of tabs won't be updated. But it does work for simple cases.

So, it's something to think about whether we want to continue supporting calling child.destroy() without doing a removeChild() first. If so maybe we'd want to connect to each child's destroy() method and then call removeChild() or something like that.

comment:12 Changed 9 years ago by bill

Resolution: fixed
Status: newclosed

(In [22269]) Remove _Container dependency on dojo.query() by removing the _Container.getChildren() method, and just defaulting to use the inherited _Widget.getChildren(). _Container.getChildren() was supposed to be faster than _Widget.getChildren() but in practice I don't think it was, or at least there wasn't an important difference.

This change will break user code that was depending on getChildren() returning a "souped-up" array, code like myWidget.getChildren().forEach(...) or myWidget.getChildren().filter(...). That code will no longer work on IE, and should be changed to dojo.forEach(widget.getChildren(), .... Technically getChildren() was always defined as returning a plain array but some may have been depending on this undocumented behavior.

I didn't use the patch in #10087 (sorry Les) for the reasons I listed in the ticket.

Fixes #10087 !strict.

Note: See TracTickets for help on using tickets.