Ticket #5796 (new defect)

Opened 10 months ago

Last modified 2 weeks ago

make destroy() work recursively

Reported by: bill Owned by: dante
Priority: normal Milestone: 2.0
Component: Dijit Version: 1.0
Severity: normal Keywords:
Cc: tk

Description (last modified by bill) (diff)

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

follow-up: ↓ 8   Changed 10 months 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.

  Changed 9 months ago by alex

  • milestone changed from 2.0 to 1.3

Milestone 2.0 deleted

  Changed 7 months ago by dante

  • cc tk added
  • owner changed from dante, tk to dante
  • description modified (diff)

  Changed 5 months ago by bill

  • description modified (diff)
  • milestone changed from 1.3 to 2.0

See also #6948.

  Changed 5 months 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...

  Changed 5 months 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();

  Changed 2 months 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

in reply to: ↑ 1   Changed 2 weeks 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.

Note: See TracTickets for help on using tickets.