_Container.addChild() tests for _started attribute

[rcoup]

dijit._Container.addChild() tests for the _started attribute on the parent widget to decide whether to call a child widgets startup() method.

16 widgets implement _Container but only 8 use _started (rough grep count).

This will only be noticeable if people are messing with containers dynamically. eg. adding a menu item programmatically after the menu is created.

Widgets that implement _Container but don't use _started:

• dijit.form.Button
• dijit.form.HorizontalSlider?
• dijit._TreeNode
• dijit.Menu
• dijit.Toolbar

(from a quick grep, may not include everything)

[bill]

I don't understand what the issue is here. There's a startup() method in Widget.js that everyone inherits. I guess that should be setting this._started to true. Is that the problem? You said that dijit.form.Button doesn't reference _started, but why do you say that it should?

[rcoup]

IMO, any widget that implements Container should use _started:

• When a child widget gets added, the child's startup() only gets called if the parent is already started (determined via _started).
• Otherwise it's assumed startup() on the parent will be called in the future and will call all the child startup methods (which is true)

Putting it into the base startup method (or in Container.startup()?) would be fine, except that widgets don't seem to call the parent startup() methods.

Menu, Toolbar, & Tree are probably the key ones since they are much more likely to have children manipulated at runtime than a button or a slider.

[bill]

I see. That sounds reasonable. OK, can you provide a patch for this? Everyone should be calling this.inherited("startup", arguments), and I guess Widget.js should set this._started=true;

[rcoup]

hrm. Slightly more complicated than I expected.

Is startup() guaranteed to only be called once? In what cases would it ever be called more than once?

A number of widgets check for it like this:

if (this._started) return;

... call child widgets and do other things ...

this._started = true;

So, we could replace that with this pattern (_started is set at the start of started()):

started : function() {
    if (!this.inherited("startup")) return false;

    ... do things ...

    return true;
}

and make startup() return false if the widget has previously been started.

or (_started is set at the end of started()):

startup : function() {
    if (this._started) return;

    ... do stuff ...
    
    this.inherited("started", arguments);
}

I'm wary of changing how startup() works for widgets where I'm not sure of the ramifications. But my vote would be for the second option.

I'm happy to prepare a patch for dijit/dojox once a call is made :)

[bill]

Replying to rcoup:

hrm. Slightly more complicated than I expected. Is startup() guaranteed to only be called once? In what cases would it ever be called more than once?

I don't remember the exact sequence here but sometimes it was being called more than once. That's why we have the _started flag.

The second option looks good to me (except you need braces on your if() statement as per dojo coding standards.

[bill]

(In [11731]) Make sure _started flag is set and checked correctly. Fixes #4920.