Forgotten removeChangeListener hard to debug and dangerous


#1

because, changeListener-Calls are asynchronous, its hard to find the source when you accidentally forgotten to remove them (from the Broadcaster) before you deleted them. Also its dangerous, because often crashes/bugs appearing only when the class is used a second time, and you don’t get any fault if you using them only one time.
I’ve got a complex bug here, where lots of class where connected to the same broadcasters, and i was unable to find which listener was the cause.
So there should be a hint if you forgot to remove the Listener (btw, i know its not always neccessary, if the Broadcaster is deleted before the listener, i found some positions in FileTreeView where you do so), but i would say its generally unsafe.

So there should be, at least, a warning if you forgot to remove a changeListener. Here is a modofied ChangeListener/Broadcaster, which checks, if all broadcaster were disconnect before deleting them.

[code]
class JUCE_API ChangeListener
{
public:
/** Destructor. */
virtual ~ChangeListener()
{
jassert(assignedBroadcasters.size()==0) // New
}

/** Your subclass should implement this method to receive the callback.
    @param source the ChangeBroadcaster that triggered the callback.
*/
virtual void changeListenerCallback (ChangeBroadcaster* source) = 0;

Array<ChangeBroadcaster*> assignedBroadcasters;

private:

friend class ChangeBroadcaster;											//new
void addBroadcasterToRemember(ChangeBroadcaster* source)				//new
{
	assignedBroadcasters.add(source);
}
void removeBroadcasterToRemember(ChangeBroadcaster* source)				//new
{
	assignedBroadcasters.removeValue(source);
}

//==============================================================================

#if JUCE_CATCH_DEPRECATED_CODE_MISUSE
// This method’s signature has changed to take a ChangeBroadcaster parameter - please update your code!
private: virtual int changeListenerCallback (void*) { return 0; }
#endif
};[/code]

void ChangeBroadcaster::addChangeListener (ChangeListener* const listener)
{
    // Listeners can only be safely added when the event thread is locked
    // You can  use a MessageManagerLock if you need to call this from another thread.
    jassert (MessageManager::getInstance()->currentThreadHasLockedMessageManager());

    changeListeners.add (listener);
	listener->addBroadcasterToRemember(this);		//New
}

void ChangeBroadcaster::removeChangeListener (ChangeListener* const listener)
{
    // Listeners can only be safely added when the event thread is locked
    // You can  use a MessageManagerLock if you need to call this from another thread.
    jassert (MessageManager::getInstance()->currentThreadHasLockedMessageManager());

	listener->removeBroadcasterToRemember(this);    //New
    changeListeners.remove (listener);
}

void ChangeBroadcaster::removeAllChangeListeners()
{
    // Listeners can only be safely added when the event thread is locked
    // You can  use a MessageManagerLock if you need to call this from another thread.
    jassert (MessageManager::getInstance()->currentThreadHasLockedMessageManager());

	Array<ChangeListener*> cl=changeListeners.getListeners();

	for (int i=cl.size()-1;  i>=0; --i)
	{
		cl.getReference(i)->removeBroadcasterToRemember(this);
	}


    changeListeners.clear();   


}

#2

+1 !


#3

oh this is an old post from me. I think my old solution doesn't handle the case if a broadcaster is delete before a listener. Generally i think the whole listener architecture should use safe-pointers.


#4

I uploaded a version with safe pointers a few weeks ago:

http://www.juce.com/forum/topic/changebroadcaster/changelistener-version-weakreferences

you will still get an assertoon if you didn't remove it, but it won't crash anymore.