Opened 12 years ago

Closed 6 years ago

#5796 closed defect (fixed)

make destroy() work recursively

Reported by: bill Owned by: dante
Priority: high Milestone: 2.0
Component: Dijit Version: 1.0
Keywords: Cc: tk
Blocked By: Blocking:

Description (last modified by bill)

Seems there's never a case for when a user wants to destroy a widget without destroying it's descendants. Thus, could get rid of the destroyRecursive() method and just make destroy() work like destroyRecursive().

Unfortunately that's an API change to destroy(), so I think we need to wait until 2.0 to do it.

Change History (22)

comment:1 Changed 12 years ago by bill

PS: this may be difficult/impossible because we call getDescendants() and then call destroy() on each widget found. If destroy() was recursive then we would be destroying nested widgets multiple times.

comment:2 Changed 12 years ago by alex

Milestone: 2.01.3

Milestone 2.0 deleted

comment:3 Changed 11 years ago by dante

Cc: tk added
Description: modified (diff)
Owner: changed from dante, tk to dante

comment:4 Changed 11 years ago by bill

Description: modified (diff)
Milestone: 1.32.0

See also #6948.

comment:5 Changed 11 years ago by alex

can we add a param to specify whether or not destorying should be recursive? That'll let us keep back-compat and let us keep the current destroy semantic...

comment:6 Changed 11 years ago by bill

Unfortunately there's already a parameter to destroy, thus adding another would make the syntax for the common case

foo.destroy(false, true);

which seems more complicated than

foo.destroyRecursive();

comment:7 Changed 11 years ago by bill

(In [15339]) More ContentPane? destruction related fixes:

  1. Emptying of contents now handled exclusively in destroyDescendants(); removed other calls to setter.empty().
  2. destroyDescendants() calls _onUnloadHandler() instead of destroy() (TODO: loading an href will call _onUnloadHandler() twice, once when the "Loading..." message is displayed and once when the actual new content is displayed)
  3. make ContentPane? override destroyRecursive() instead of overriding destroy(), since the standard way to destroy widgets is destroyRecursive() (see #5796)
  4. comments, plus mising semicolon

Fixes #7727 !strict

comment:8 in reply to:  1 Changed 11 years ago by bill

Replying to bill:

PS: this may be difficult/impossible because we call getDescendants() and then call destroy() on each widget found. If destroy() was recursive then we would be destroying nested widgets multiple times.

As of [15607] and [15616] (changes to _Widget.js), and related changes to ContentPane, destroyDescendants() no longer calling destroy(). It now calls destroyRecursive() on direct descendants. So, this problem is fixed.

To implement this ticket though, this code in manager.js needs to change:

dojo.addOnWindowUnload(function(){
	dijit.registry.forEach(function(widget){ widget.destroy(); });
});

We should extract _Widget._getDescendantsHelper() into a general utility function, since it can be used to find all top level widgets under <body>, and then we can call destroyRecursive() on each of them.

comment:9 Changed 10 years ago by matthw

As someone writing long-running, single-page apps, this is quite a source of pain for me in dijit._Widget, and I'd love to see _Widget.prototype.destroy cleaned up.

I expect destroy to work recursively, but failing that, when I see a method called destroyRecursive, I really really expect this, at least, to work recursively. It doesn't at present, only calling destroy on its one-level-deep descendants, when I would expect it to call destroyRecursive in turn on all its immediate children.

There is also the issue that destroyDescendants doesn't even call destroy on all immediate children, only on children of containerNode! To my mind this distinction between descendants (which must be children of containerNode) and other child widgets (eg widgets in the template, or other widget slots where the parent creates and places the child programmatically) is not particularly helpful and further clouds the issue of correct widget finalisation.

Ontop of all this further complicating things, there's a special private facility for destroying "_supportingWidgets", which only seems to destroy widgets which were instantiated directly from the template. If you programmatically create your own child widgets for certain slots rather than using _Templated and the parser, there's no way to have them take advantage of this facility.

I frequently have to override the 'uninitialize' hook to properly destroy child widgets which aren't destroyed by destroy() or destroyRecursive().

This stuff is really key to avoiding memory leaks in long-running applications.

comment:10 Changed 10 years ago by matthw

Re correct recursive behaviour of destroyRecursive, my apologies, see this has been fixed now.

This does help, although the limiting of getDescendants to containerNode still limits the usefulness of destroyRecursive.

comment:11 Changed 10 years ago by bill

getChildren() and destroyChildren() purposefully are limited to containerNode, to correspond to addChild() and removeChild().

However, destroy() does destroy all the widgets associated with a given widget, even those outside of containerNode, because those widgets outside of containerNode are stored in this._supportingWidgets, which is dealt with in _Widget.destroy(). If you are creating widgets outside of containerNode manually (ie, not from the widgetsInTemplate feature), then you should add them to _supportingWidgets.

comment:12 Changed 10 years ago by bill

Oops, I meant to say destroyRecursive() / destroyDescendants() above, not destroyChildren() (which isn't a method at all).

comment:13 Changed 10 years ago by Eugene Lazutkin

Note: dojox/lang/oo/declare.js (http://bugs.dojotoolkit.org/browser/dojox/trunk/lang/oo/declare.js) --- a drop-in replacement for the venerable dojo.declare() --- introduces makeDeclare(before, after), which creates a specialization of declare to automate method chaining.

Typical pattern is to chain life-cycle events, e.g., constructors are chained in the "after" fashion, so innermost constructors are executed first, while destructors are chained in the "before" fashion --- effectively in the reverse order of constructors. By default only constructors are chained. With the new facility now we can do something like that:

var oo = dojox.lang.oo;
dijit.declare = oo.makeDeclare(["destroy"], ["constructor"]);

The resulting dijit.declare() still compatible with dojo.declare() but chains listed methods automatically.

Any classes created with dijit.declare() will automatically chain destroy() using the "before" algorithm. Widgets have a bunch of life-cycle methods, e.g., postMixInProperties, buildRendering, postCreate, and multiple destroyXXX methods, which can benefit from the automated chaining. That way we can throw away the recursive boiler plate, cut down on unintentional mistakes, and improve the performance.

I am not sure that this change can be done in 1.4, but probably we should think about taking advantage of this possibility for 2.0.

comment:14 in reply to:  13 ; Changed 10 years ago by Ben Lowery

Replying to elazutkin:

Typical pattern is to chain life-cycle events, e.g., constructors are chained in the "after" fashion, so innermost constructors are executed first, while destructors are chained in the "before" fashion --- effectively in the reverse order of constructors. By default only constructors are chained. With the new facility now we can do something like that:

I think you've got that backwards. You'd want the outermost constructors to run first and the innermost destructors to run first. That way derived classes can make some level of assumption about what's available to play with and modify. For example, if I know my base class sets this.foo = 5 and I want this.foo = 10, my ctor needs to run after my base's ctor.

comment:15 in reply to:  14 ; Changed 10 years ago by Eugene Lazutkin

Replying to blowery:

I think you've got that backwards. You'd want the outermost constructors to run first and the innermost destructors to run first. That way derived classes can make some level of assumption about what's available to play with and modify. For example, if I know my base class sets this.foo = 5 and I want this.foo = 10, my ctor needs to run after my base's ctor.

With all due respect I think you've got that backwards. :-) For example, your 1st sentence contradicts your 3rd sentence.

Or maybe your definition of "outermost" and "innermost" is different from what I assume. To me any base class will be an "inner" object. Or you interpret the meaning of "after" and "before" advices differently.

But your 3rd sentence state that "my ctor needs to run after my base's ctor" --- compare it with mine: "constructors are chained in the "after" fashion, so innermost constructors are executed first" (emphasis mine). So basically we said the same --- the constructors are executed in the depth-first manner going from base classes to derived classes. Derived classes envelop base classes constructing a structure similar to matryoshka (or onion) where base classes are inner layers, and a most derived class is an outer layer.

So I am confused now, and waiting for your clarifications of the terminology you use, preferably with references to authoritative sources (e.g., Wikipedia is just fine).

comment:16 in reply to:  15 Changed 10 years ago by Ben Lowery

Replying to elazutkin:

Replying to blowery:

I think you've got that backwards. You'd want the outermost constructors to run first and the innermost destructors to run first. That way derived classes can make some level of assumption about what's available to play with and modify. For example, if I know my base class sets this.foo = 5 and I want this.foo = 10, my ctor needs to run after my base's ctor.

With all due respect I think you've got that backwards. :-) For example, your 1st sentence contradicts your 3rd sentence.

Or maybe your definition of "outermost" and "innermost" is different from what I assume. To me any base class will be an "inner" object. Or you interpret the meaning of "after" and "before" advices differently.

Ah, that's it. I consider outermost to be the base and the innermost to be the most derived. So if you had C derive from B derive from A, C would be the innermost, A would be the outermost. I would expect A's ctor to run first, C's last. So I think we're in agreement. :)

comment:17 Changed 10 years ago by Les

Seems there's never a case for when a user wants to destroy a widget without destroying it's descendants.

Let's say I have a TabContainer? container containing a single tab. This could happen b/c the user closed a bunch of tabs and there's only one left. If the user closes the last tab, I'd like to destroy the TabContainer?, but save the content of the last tab, which I'd like to insert into the ContentPane? intially containg the TabContainer?. So, here's a case where I'd like to destroy the container w/o destroying its content. I guess I could detach the domNode of the last ContentPane? and this would prevent it from being destroyed.

comment:18 Changed 9 years ago by dante

@bill - I have a function I use to do this, in a 1.x - compat way:

dijit.kill = function(widget, preserve){
   // summary: Kill off this and everyone under me
   if(widget){
      if(widget.destroyRecursive){
         widget.destroyRecursive(preserve);
      }else if(widget.destroy){
         widget.destroy(preserve);
      }
   }
};

for 1.x this will suffice until we can normalize the behavior in 2.x?

comment:19 in reply to:  17 Changed 9 years ago by dante

Replying to Les:

Seems there's never a case for when a user wants to destroy a widget without destroying it's descendants.

Let's say I have a TabContainer? container containing a single tab. This could happen b/c the user closed a bunch of tabs and there's only one left. If the user closes the last tab, I'd like to destroy the TabContainer?, but save the content of the last tab, which I'd like to insert into the ContentPane? intially containg the TabContainer?. So, here's a case where I'd like to destroy the container w/o destroying its content. I guess I could detach the domNode of the last ContentPane? and this would prevent it from being destroyed.

removeChild should handle this before you destroy it. Eg: you shouldn't be destroy()'ing it at that point, you should be removing it from whatever layout it is in. If that doesn't work we have a problem.

comment:20 Changed 8 years ago by bill

In [27544]:

Document uninitialize() as deprecated. Widget code can simply override destroy() instead. Refs #5796 !strict.

comment:21 Changed 7 years ago by bill

In [30464]:

add deprecation-type comment, refs #5796

comment:22 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

Implemented in our 2.0 prototype so closing this ticket.

Note: See TracTickets for help on using tickets.