Listener Coding Standards

Inspired by the Coding Standards http://www.rawmaterialsoftware.com/wiki/index.php/Coding_Standards, it would be nice to have some rules how to use listeners.
Because I often add listeners to objects which have an independent lifetime, I’m often not feeling comfortable about doing this.
(and the more the rules are getting to complicated, this can be a sign that the listener/broadcaster mechanism is to complicated in some circumstances (http://www.rawmaterialsoftware.com/viewtopic.php?f=2&t=6401)

no guarantee:

If I’m a class with a ListenerCallback and want to listen to another class like: otherclass->addListener(this)

if otherclass is ALWAYS deleted BEFORE me (if it’s a child/member of me)

  • i don’t have to do unregister myself (but I should do it when I’m getting destroyed to be clean)

but if otherclass is ALWAYS deleted AFTER me (if its my parent/I’m a member of otherclass)

  • i do have to unregister myself, when I’m deleted

if otherclass has a independent lifetime and can be deleted BEFORE or AFTER me

  • i have to use something like WeakPointer and watch its lifetime, and if its still living when i’m getting destroyed, i have to unregister myself

If I’m a broadcaster and want that another class is listening to me: addListener(otherClass)

if otherclass is ALWAYS deleted BEFORE me (like if is a child/member of me)

  • i have to do unregister it (before its deleted)

but if otherclass is ALWAYS deleted AFTER me (if its a parent/I’m a member of otherclass)

  • i don’t have to unregister it (but I should)

if otherclass has a independent lifetime and can be deleted BEFORE or AFTER me

  • we have a problem, because every time I broadcast a change, i have to be sure its still living

Just my opinion but I think this behavior should be strongly discouraged. It creates an unnecessary dependency of the broadcaster on the listener, which totally defeats one of the elegant features of using the listener in the first place: That the broadcaster doesn’t care how many or what particular listeners are attached.

Correct me if I’m wrong but I think juce Listeners work this way:

[1] Anyone can add themselves as a Listener at any time.
[2] A Listener must remove itself before the broadcasting object is destroyed.
[3] A previously added Listener may be removed at any time.
[3.1] A Listener may remove itself from within a Listener callback.
[3.2] It is safe to remove any installed listener from within a Listener callback.
[4] The Listener callback is made from the thread of the broadcaster.

For #2 this was the subject of a lively discussion regarding the implementation of a “ScopedListener” (here: http://rawmaterialsoftware.com/viewtopic.php?f=2&t=6401), designed to ease lifetime management for attached objects. Unfortunately this won’t work with some complex concurrency models but it was a nice idea.scope

My entire application is built on the Listener concept (except that Listeners can specify on which thread they want to be notified, a non trivial implementation). They are extremely robust and in my opinion it is the superior model for implementing the type of concurrent systems typically encountered in the development of audio applications or live performance tools. Listeners can be thought of as a very simplified form of MPI (Message Passing Interface) which works between threads instead of between processes:

Just my opinion but I think this behavior should be strongly discouraged. It creates an unnecessary dependency of the broadcaster on the listener, which totally defeats one of the elegant features of using the listener in the first place: That the broadcaster doesn’t care how many or what particular listeners are attached.[/quote]

I think you’ve missed his point there (and read some other meaning into his words), because he’s simply describing a broadcaster/listener relationship. I.e. in the first section he’s saying “for listeners…”, and then “for broadcasters…”.

I disagree, in his second code snippet “otherClass” is clearly the listener, and the call to addListener is implicitly “this->addListener(…)”. Because otherwise only the first example is needed. He then goes on to talk about the broadcaster needing to know about lifetime management of the listener, which lends further support to my interpretation. Of course if chkn wants to clear this up that would be best :slight_smile:

But regardless, I still assert that having broadcasters manually add known listeners is a bad design.

[quote=“chkn”]If I’m a broadcaster and want that another class is listening to me: addListener(otherClass)if otherclass is ALWAYS deleted BEFORE me (like if is a child/member of me)

  • i have to do unregister it (before its deleted)

but if otherclass is ALWAYS deleted AFTER me (if its a parent/I’m a member of otherclass)

  • i don’t have to unregister it (but I should)

if otherclass has a independent lifetime and can be deleted BEFORE or AFTER me

  • we have a problem, because every time I broadcast a change, i have to be sure its still living[/quote]

This is exactly why the responsibility for registering and unregistering a listener should fall squarely on the object which is listening and not the broadcaster.

the second example describes that I’m the broadcaster and otherclass is the listener, this->addListener(otherclass), and i thing regardless if i’m the listener or broadcaster, i think, who is adding the relationship, should also have the responsibility to care. You may say its a bad design, and you probably right, but it is wrong?

There are some places where in the JUCE codebase where a listener is added to a broadcaster, not by itself. Is it a bad-design and why? (Thats why i want to have some rules here, or new Listener Design which cares about lifetimes).

src\gui\components\keyboard\juce_KeyMappingEditorComponent.cpp(440)

[code]KeyMappingEditorComponent::KeyMappingEditorComponent (KeyPressMappingSet& mappingManager,
const bool showResetToDefaultButton)
: mappings (mappingManager),
resetButton (TRANS (“reset to defaults”))
{
treeItem = new TopLevelItem (*this);

if (showResetToDefaultButton)
{
    addAndMakeVisible (&resetButton);
    resetButton.addListener (treeItem);  // <-----------------------------------------
}

addAndMakeVisible (&tree);
tree.setColour (TreeView::backgroundColourId, findColour (backgroundColourId));
tree.setRootItemVisible (false);
tree.setDefaultOpenness (true);
tree.setRootItem (treeItem);

}[/code]

src\gui\components\layout\juce_TabbedButtonBar.cpp(371):

[code]void TabbedButtonBar::resized()
{
int depth = getWidth();
int length = getHeight();

if (orientation == TabsAtTop || orientation == TabsAtBottom)
    std::swap (depth, length);

const int overlap = getLookAndFeel().getTabButtonOverlap (depth)
                        + getLookAndFeel().getTabButtonSpaceAroundImage() * 2;

int i, totalLength = overlap;
int numVisibleButtons = tabs.size();

for (i = 0; i < tabs.size(); ++i)
{
    TabBarButton* const tb = tabs.getUnchecked(i)->component;

    totalLength += tb->getBestTabLength (depth) - overlap;
    tb->overlapPixels = overlap / 2;
}

double scale = 1.0;

if (totalLength > length)
    scale = jmax (minimumScale, length / (double) totalLength);

const bool isTooBig = totalLength * scale > length;
int tabsButtonPos = 0;

if (isTooBig)
{
    if (extraTabsButton == nullptr)
    {
        addAndMakeVisible (extraTabsButton = getLookAndFeel().createTabBarExtrasButton());
        extraTabsButton->addListener (behindFrontTab);      // <------------------------------------------------------------------
        extraTabsButton->setAlwaysOnTop (true);
        extraTabsButton->setTriggeredOnMouseDown (true);
    }

    const int buttonSize = jmin (proportionOfWidth (0.7f), proportionOfHeight (0.7f));
    extraTabsButton->setSize (buttonSize, buttonSize);

    if (orientation == TabsAtTop || orientation == TabsAtBottom)
    {
        tabsButtonPos = getWidth() - buttonSize / 2 - 1;
        extraTabsButton->setCentrePosition (tabsButtonPos, getHeight() / 2);
    }
    else
    {
        tabsButtonPos = getHeight() - buttonSize / 2 - 1;
        extraTabsButton->setCentrePosition (getWidth() / 2, tabsButtonPos);
    }

    totalLength = 0;

    for (i = 0; i < tabs.size(); ++i)
    {
        TabBarButton* const tb = tabs.getUnchecked(i)->component;

        const int newLength = totalLength + tb->getBestTabLength (depth);

        if (i > 0 && newLength * minimumScale > tabsButtonPos)
        {
            totalLength += overlap;
            break;
        }

        numVisibleButtons = i + 1;
        totalLength = newLength - overlap;
    }

    scale = jmax (minimumScale, tabsButtonPos / (double) totalLength);
}
else
{
    extraTabsButton = nullptr;
}

int pos = 0;

TabBarButton* frontTab = nullptr;

for (i = 0; i < tabs.size(); ++i)
{
    TabBarButton* const tb = getTabButton (i);

    if (tb != nullptr)
    {
        const int bestLength = roundToInt (scale * tb->getBestTabLength (depth));

        if (i < numVisibleButtons)
        {
            if (orientation == TabsAtTop || orientation == TabsAtBottom)
                tb->setBounds (pos, 0, bestLength, getHeight());
            else
                tb->setBounds (0, pos, getWidth(), bestLength);

            tb->toBack();

            if (i == currentTabIndex)
                frontTab = tb;

            tb->setVisible (true);
        }
        else
        {
            tb->setVisible (false);
        }

        pos += bestLength - overlap;
    }
}

behindFrontTab->setBounds (getLocalBounds());

if (frontTab != nullptr)
{
    frontTab->toFront (false);
    behindFrontTab->toBehind (frontTab);
}

}[/code]

In your example the TopLevelItem is part of the class composition of KeyMappingEditorComponent. So I would consider them to be a single unit.