MessageManager Addition

[mattsonic]

Hey Jules,

What do you think about adding a getter to the MessageManager class so that users can read the messageListeners array?

I just had to debug problems related to this extremely useful assertion:

Code: Select all

MessageManager::~MessageManager() throw()
{
    ...
    jassert (messageListeners.size() == 0);


I ended up writing a very short getter in juce_MessageManager.h so that I could iterate through the messageListeners array and determine which components were leaking:

Code: Select all

   SortedSet <const MessageListener*> getMessageListeners() {
      return messageListeners;
   }


What do you think about adding this? It was extremely useful to help identify leaking components.

It allowed me to write a quick snippet in my app's shutdown() method to list all the leaking components:

Code: Select all

   SortedSet <const MessageListener*> messageListeners = MessageManager::getInstance()->getMessageListeners();
   for (int i = 0; i < messageListeners.size(); i++) {
      const MessageListener* messageListener = messageListeners[i];
      const Component* component = dynamic_cast<const Component*> (messageListener);
      if (component != 0) {
         // No components should exist at this point, so if they do then they leaked
         String parentName;
         Component* parentComponent = component->getParentComponent();
         if (parentComponent != 0) {
            parentName = parentComponent->getName();
         }
         DBG("JuceApp::shutdown(): Leaked component " + String(i) + " found: "
            + component->getName() + " Parent: " + parentName)
      }
   }


Matt Sonic

[jules]

You're a bit late with that, I'm afraid - I've just been refactoring the way components work, and they're no longer MessageListeners, so your cunning trick wouldn't work any more.. But I also added an assertion that fires when you leak components too, so you probably wouldn't need it anyway!

[chkn]

jules, i see you changed the whole MessageListener/brodcaster stuff, i know you have your reasons for this, but now i have a very big problem.

I used my elegant and simple MessageHub (see below) in that way, that my GUI-Components (many) can self register to the Hub.
The Hub is registered as a message Listener of my document classes (many).

This has the advantage that i can replace the Document-Classes or Component-Classes, without reconnect them again (many Connections)
Also i can create Components later and i only have to connect them with the Message hub.

At least i need something like sendSynchronousChangeMessage with the old behavior, where you can define a pointer as addressor.

Code: Select all

class MessageHub : public ChangeListener, public ChangeBroadcaster
{
public:
  void changeListenerCallback( void* objectThatHasChanged )
  {
   sendSynchronousChangeMessage(objectThatHasChanged);
  }
};

[jules]

Part of my reasons for changing the ChangeBroadcaster is that what you're doing there would never have worked properly anyway. If you were to make two calls in succession with different pointers, e.g. sendChangeMessage (pointer1); sendChangeMessage (pointer2); then only one of those pointers would actually be delivered to the listeners, and the other would be lost.

[chkn]

Code: Select all

that what you're doing there would never have worked properly anyway.

it works, and it works very good and stable, because i know what i'm doing (please believe me)

Code: Select all

and the other would be lost.

no in the MessageHub is use sendSynchronousChangeMessage which call the the listeners directly for this reason (because its runs in a already asynchronous callback)

the point is, i only need my old sendSynchronousChangeMessage(objectThatHasChanged) -method back, or something equivalent

please

[jules]

Ok, I see what you mean, but a ChangeBroadcaster is the wrong tool for the job you're trying to do.

If your hub object always calls the listeners synchronously, then it should just manage and call its listeners with a ListenerList<ChangeListener>. Unless you want async messages, there's no reason to use a ChangeBroadcaster.

[mattsonic]

You're a bit late with that, I'm afraid - I've just been refactoring the way components work, and they're no longer MessageListeners, so your cunning trick wouldn't work any more.. But I also added an assertion that fires when you leak components too, so you probably wouldn't need it anyway!

Ahh even better. I must get everything up to date with the latest tip, it seems. Thanks!

[chkn]

manage and call its listeners with a ListenerList<ChangeListener>

Thanks! ListenerList works for me!

[Bruce Wheaton]

This thread looked good for a while, then my hopes were dashed.

I'm hitting the leaked listener assertion on shut-down, and was very confused until you changed the comments Jules, and lead me to believe the leaks are AsyncUpdaters being held by threads that haven't finished shutting down.

Is there a trick that still would show me what is still alive? Then I have a chance of seeing what thread is still running or holding them.

Bruce

[jules]

Hmm, the comment above the assertion in ~MessageManager is actually out-of-date now, because I've just changed AsyncUpdater and ChangeBroadcaster to no longer inherit from MessageListener (in fact there are very few classes left that still use MessageListeners). Are you using the very latest version?

[Bruce Wheaton]

Very tippy top. I changed when I hit the Resizeable crash.

I guess I actually do have MessageListeners leaking then. I'd guess they're in threads, but since I don't make many Message Listeners myself, I'm a tiny bit at sea.

So, to step back one, should I actually try to stop all threads (and wait for them) before I let this part of shutdown complete? It occurs to me that Juce and/or the OS is probably forcibly killing the threads around this time anyway.

Bruce

[jules]

Yes, you should certainly try to get all your threads to shutdown cleanly.

[halm]

Totally new to Juce, so forgive me if this is the wrong place or is exceedingly stupid.

I've inherited quite a large Juce application that worked fine with 1.50. One of the things I'm tasked with right now is moving to 1.52.

The application has a Component with a Slider, with this:

Code: Select all

...
//ctor:
{
  addAndMakeVisible( slider1 = new Slider( T("mySlider1")) );
  ...
  slider1->addListener(this);
  addAndMakeVisible( slider2 = new Slider( T("mySlider2")) );
  ...
  slider2->addListener(this);
}

...
void MySliderComponent::sliderValueChanged( Slider * sliderThatWasMoved )
{
  if( sliderThatWasMoved == slider1 )
  { ...
    sendSynchronousChangeMessage( new Message( param, Slider1Enum, 0, this );
  }
  else if( sliderThatWasMoved == slider2 )
  { ...
    sendSynchronousChangeMessage( new Message( param, Slider1Enum, 0, this );
  }


There is a MainComponent class which contains pointers to a number of sliders, like the one above.

Code: Select all

void MainComponent::changeListenerCallback(ChangeBroadcaster * objectthatHasChanged)
{
Message * message = (Message *)objectThatHasChanged;
String name      = ((Component *)message->pointerParameter)->getName();
int param1   = message->intParameter1;
int param2   = message->inParameter2;
  if( 0 == name.compare( T("mySlider1")) )
  { // do something
  }
  else if( 0 == name.compare( T("mySlider2")) )
  { // do something else
  }
...
}


I'm only just starting to learn Juce, and I've read the discussion here. sendSynchronousChangeMessage() no longer accepts an argument that I can use to retrieve parameters.
I've considered putting the data in my Message into member variables. Since messages are only sent synchronously this should be thread safe, though inelegant.

If I understood the suggestion in one of the discussions properly, ListenerList is recommended instead. This doesn't really seem equivalent to me and would require a fairly significant refactoring of my application. Before I undertake it I want to see if that is indeed the preferred method. Also, are calls to the call() function synchronous?

[jules]

Wow. You have my sympathy - that looks like quite a heap of crap you've inherited. Only a special kind of programmer would choose to write:

Code: Select all

  if( 0 == name.compare( T("mySlider1")) )


..instead of

Code: Select all

  if (name == "mySlider1")


And they're casting a ChangeBroadcaster to a Message object!??

And what happens to that original Message object that gets allocated, won't it leak..?

1.50 is so long ago that I can't remember exactly how sendSynchronousChangeMessage used to work, but based on this code snippet, I can't see any good reason for trying to use ChangeBroadcaster here - the class is totally inappropriate for this purpose. It looks like they're just using sendSynchronousChangeMessage as an incredibly clunky (and probably leaky) way of calling the changeListenerCallback, so I would definitely recommend replacing it with a ListenerList instead, which does work synchronously, and would be a vastly cleaner way of doing it.