All classes should remove listeners properly #103

Open
jhnns opened this Issue Oct 3, 2012 · 8 comments

Comments

Projects
None yet
3 participants
Owner

jhnns commented Oct 3, 2012

I stumbled upon line 222 of the ModelCollection.

//Clean in any case the listener for delete.
modelToDelete.removeAllListeners("delete", onDeleteListener);

There is no method removeAllListeners() with two arguments. removeAllListeners() removes all listeners of the given type.

I think now is the time to go over our current classes and check if all listeners are removed properly. For every on(type, func) there should be at least one removeListener(type, func), maybe more. But the class should only remove those listeners that have been added by itself.

To accomplish this it's useful to search for occurences of on(. After that we have to trace all actions that make this binding between emitter and listener invalid. For instance dispose(). Or in the ModelCollection: The change event listener needs to be removed too when the model fires a delete event.

I'm assigning it to @topa, so he can refactor it. After that @meaku should go over his classes.

topa was assigned Oct 3, 2012

Owner

jhnns commented Oct 3, 2012

When applicable we should add a check to the unit test if all listeners have been removed.

@jhnns jhnns added a commit that referenced this issue Oct 3, 2012

@jhnns jhnns - Fixed removeAllListeners (described in #103)
- Fixed bug when several models are removed via delete-event
617de8b
Owner

topa commented Oct 4, 2012

I'll take care of these classes:

  • App 👍
  • DisplayObject 👍
  • Page 👍
  • PageLoader 👍
  • View 👍
  • ViewCollection 👍
  • Collection 👍
  • ModelCollection 👍

meaku was assigned Oct 4, 2012

Owner

jhnns commented Oct 4, 2012

Cool!

Owner

topa commented Oct 5, 2012

The DisplayObjects provides a method to bind events it node and node's sub-node. But the only possibility to remove this events is to call .dispose() or require the beloved domAdapter and use it's .off. I think we should provide an additional method on the DisplayObject for this.
@jhnns what do you think (new issue)?

@topa topa pushed a commit that referenced this issue Oct 5, 2012

topa - ModelCollection's .dispose() isn't disposing Models anymore, but re…
…moves all listeners from itself and sets inner collection to null. #103
9334d31
Owner

topa commented Oct 5, 2012

done()

Owner

jhnns commented Oct 5, 2012

We may provide this in the future but for now it's not urgent.

If the developer wants to remove event listeners from DOM nodes he should use his DOM framework. The DisplayObject is only providing addNodeEvents because if the DisplayObject is disposed, all DOM events should be removed. We don't want to replace tasks that are usually done by a DOM framework.

@topa topa pushed a commit that referenced this issue Oct 5, 2012

topa - using now removeListener for undelegation onAdd #103 3a9af97
Owner

meaku commented Jan 10, 2013

  • Model
  • View
  • Page
Owner

jhnns commented Feb 18, 2013

I'll leave this open. We should add unit tests for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment