Opened 13 years ago

Closed 12 years ago

#3050 closed defect (fixed)

0.9: Widget cleanup and destroy functions

Reported by: Jared Jurkiewicz Owned by: bill
Priority: high Milestone: 0.9beta
Component: Dijit Version: 0.9
Keywords: Cc:
Blocked By: Blocking:

Description

When looking at widget cleanup and destroy functions for the widget, it is not obvious how to go about doing a custom destroy function that will ensure specially allocated things, such as topic subscriptions or programatically created composite child widgets are properly cleaned up.

In 0.4.2, you would do so by implementing destroy() on your widget, in 0.9, destroy exists and does things like cleaning up connected events should you connect via the widget connect() function. But, in looking further at manager.js, when destroyAll is called, it only calls _destroy(), and _destroy does not call destroy(). So, if you put all your cleanup in destroy over-ride, and you do a page reload, manager isn't going to invoke it and your cleanup just sin't going to happen (as far as I can tell).

So, I guess the question/bug here is, is should manager.js' destroyAll() be calling destroy instead of _destroy() (Since destroy's default impl called _destroy anyway())? Is it expected that widget implementors should over-ride both destory and _destroy? If so, why is _destroy private?

In other words, what is the expected pattern for custom cleanup and is there a problem here in dijit? At a first glance, it looks like there is a problem to me.

Change History (4)

comment:1 Changed 13 years ago by bill

Widget implementers should override _destroy() not destroy(). Perhaps I should rename destroy --> destroyRecursive and _destroy --> destroy, to be less confusing for widget implementers, but I think that would be more confusing for widget users, who just want to call destroy().

I will update the doc for _destroy() to say "Widgets should override this function (not destroy()) to do custom cleanup".

Anything else to make it more clear?

comment:2 Changed 13 years ago by Jared Jurkiewicz

I think not having the function as _ (implicit private) would help. I like your first idea the best (destroyRecursive(), and destroy(), instead of destroy() and _destroy(). I think that makes it much more intuitive.

But as long as the doc was very explicit/clear on it, then over-riding _destroy is probably okay, I guess. :-)

comment:3 Changed 13 years ago by bill

Status: newassigned

comment:4 Changed 12 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [8744]) Rename _destroy() to destroy(), and destroy() to destroyRecursive(). Fixes #3050.

Note: See TracTickets for help on using tickets.