Opened 9 years ago

Closed 7 years ago

#11220 closed defect (patchwelcome)

[patch][cla] dojox.dtl.contrib.dijit - widgets are not destroyed

Reported by: ben hockey Owned by: ben hockey
Priority: high Milestone: 1.9
Component: DojoX DTL Version: 1.5.0b2
Keywords: Cc:
Blocked By: Blocking:

Description

a widget created using dojox.dtl._DomTemplated does not destroy it's child widgets because the widgets are not added to the array of _supportingWidgets. i have attached a patch that attempts to address this. i would appreciate comments and feedback.

Attachments (1)

dtl-dijit.diff (1.7 KB) - added by ben hockey 9 years ago.

Download all attachments as: .zip

Change History (8)

Changed 9 years ago by ben hockey

Attachment: dtl-dijit.diff added

comment:1 Changed 9 years ago by Neil Roberts

Status: newassigned

Widgets are meant to remain in memory in case they can be reused. The point of the unrender function is simply for things like if blocks where a widget might appear then disappear based on a condition (unrender called as it is no longer in the if block).

This needs a different sort of fix. _DomTemplated's destroy method should make sure any created widgets are destroyed.

comment:2 in reply to:  1 Changed 9 years ago by ben hockey

Replying to pottedmeat:

Widgets are meant to remain in memory in case they can be reused. The point of the unrender function is simply for things like if blocks where a widget might appear then disappear based on a condition (unrender called as it is no longer in the if block).

i see - so there's no need for any of the code added in unrender. makes sense.

This needs a different sort of fix. _DomTemplated's destroy method should make sure any created widgets are destroyed.

how can _DomTemplated know which widgets need to be destroyed? obviously, we can't query the DOM to find the widgets because some widgets might be unrendered and those wouldn't be found. adding the widgets to the array of _supportingWidgets doesn't quite seem right but it is effective for destroying widgets created via _DomTemplated since dijit._Widget::destroy would take care of destroying those widgets but it also means that if dijit._Widget takes a different approach towards destroying it's child widgets then dtl would need to be updated.

do you think that render.dom.Render (and DomTemplate? and the nodes in the template) should have a destroy function that can be called by _DomTemplated's destroy function? this would mean that the destroying is independent of how dijit._Widget destroys widgets and DomTemplates? which were not created as part of _DomTemplated would also provide a way to destroy their widgets. if you liked this approach then _DomTemplated would have a destroy function something like this:

destroy: function(){
    this._render.destroy();
    this.inherited(arguments);
}

apart from the _supportingWidgets approach or the destroy method for the renderer (and template and nodes) i can't really think of another way to do this. i don't mind trying to work on the solution if you want to suggest how i should approach this.

comment:3 Changed 9 years ago by Neil Roberts

This can be done through simply adding another method to the nodes like the unrender method that's notified on destruction. I'm updating the architecture and will try to remember to get this in.

comment:4 in reply to:  3 Changed 9 years ago by ben hockey

Replying to pottedmeat:

This can be done through simply adding another method to the nodes like the unrender method that's notified on destruction.

yes. that's what i was trying to describe above.

I'm updating the architecture and will try to remember to get this in.

since you're updating the architecture, just wanted to make sure that you've seen #11105 as well. the get method in the context clashes with the get method from dijit._Widget.

there is also another problem where dojoAttachEvent doesn't work for a parsed widget - see #11231

...and i'm hoping the update will make dtl compatible with django 1.2 syntax - let me know if you want some help.

comment:5 Changed 7 years ago by bill

Too bad this never got in. In 1.8, I think your entire original patch can be replaced by a simple this.own() statement on the result from the parser.instantiate() call:

this.own(this._dijit = Parser.instantiate([cloneNode(node)])[0]);

But not sure that addresses the issues Neil mentioned above.

Last edited 7 years ago by bill (previous) (diff)

comment:6 Changed 7 years ago by dylan

Milestone: tbd1.9
Owner: changed from Neil Roberts to ben hockey
Status: assignedpending

Ben, care to update your patch to get DTL support cleaned up a bit for 1.9? If not, then let's close as patchwelcome.

comment:7 Changed 7 years ago by ben hockey

Resolution: patchwelcome
Status: pendingclosed

i lost hope for dtl a long time ago so i'll leave it to someone else to put time into it if they really have a need.

Note: See TracTickets for help on using tickets.