Access to listeners

General JUCE discussion
1

Hi,

Currently doing a 5mn work that ends as a half a day work, I’m wondering about few fixes that might simplify our work in similar case.
For example, I’m using a +/- slider where I’m changing the “interval” step. However, my specific problem requires that the value are kept full resolution (so “inc step” with an interval of 10 applied to a value of 5 should result in 15, not 20).
I’ve subclassed the slider class, but due to overzealous Pimpl protection, I can’t change anything here without rewriting the whole stuff (which is counter productive).
I think PImpl is good, and I don’t think exposing it would help much, but typically, the component introspection API should be complete, so I can “simulate” the interval myself.
It’s currently lacking listener manipulation.

So I’ve tried to remove the PImpl’s buttonListener to add my own, but even that, I can’t do it.

I’ve seen multiple change to the listener code in the past few month, but I wonder if a template would be more useful here, so every listener’s caller class have the same interface, something like this:

template <class BaseClass>
class ListenerAccepter 
{
private:
    // The actual listener list
    ListenerList <Listener> listeners;

public:
    // Inject the internal Listener class inside us, so we can use it directly 
    typedef BaseClass::Listener Listener;
    
    void addListener (Listener* newListener);
    void removeListener (Listener * newListener);
    int getListenersCount(); // Added for introspection
    Listener * getListener(int index); // Added for introspection
};

The pro, well, is obvious, the interface is the same, so you never have to check the doc to find out if a particular method is supported, and introspection will be possible completely at the “Component” level.
The con, is that the documentation will be less specific for each class, but since the class will derive from the ListenerAccepter, it’ll be visible in the documentation anyway.

2

yeah… I’ve had the same thought myself, but decided not to do it mainly because I like the way that addListener methods are easy to see, and can have more specialised comments. Sure, you can find base class methods via doxygen, but it’s not as obvious, and if you’re just looking at the headers directly (which is what I do all the time, and probably lots of other people too), then it’d be easy to miss the listeners.

Another less “inheritancey” approach might be to expose some kind of public member object that does this job, e.g.

[code]class MyWidget
{
public:
ListenerManager listeners;
};

MyWidget w;
w.listeners.add (this);
[/code]

3

Not sure it’s better through, since you might still miss it while reading the doc or the header (since it’ll be in it own section as well).

Well, I least you agree with the idea, so I’ve thought about this possible improvement:

  • Move the listener documentation in a @group, an link with this group when you declare the “Listener” subclass, so you’ll get a href in the documentation at the right place
  • Write “// This component is a Listener accepter” above or below the “Listener” subclass declaration.
  • Or write a “addListener” method again, documenting it (in a #ifdef DOXYGEN section so it doesn’t break at link time)
  • If you’re using a ListenerManager or a ListenerBase, you gain a “one interface fits all” advantage, so you’re more likely not needing to read the documentation more than 2 or 3 times anyway.

I think the ListenerBase is better because it won’t break existing code (except for this, it’s probably the same if it’s a member or a base class for the mem layout, and Juce seems to prefer the inheritance approach too).

Back to the stupid initial request, what do you think about adding “getListener(i)” method to the listener managers ?

4

Well… It seems like a simple request, but listener lists are more complicated than just having an array of pointers… When you’re iterating the list, it’s not uncommon for each call to a listener to actually modify the list that’s being iterated, so that all needs to be taken into account. Some classes may also have threading issues… I’d really rather not provide any more access unless it’s unavoidable.

Could you do what you need with a more limited API, e.g. by adding a method that invokes a callback on the whole list, or returns a bool to say whether any listeners are currently registered?

5

Or how about a function that returns an array representing a “snapshot” of all the currently installed listeners?

6

That, (and/or any of these other options) would be fine for some classes, but could be problematic for others… That’s why I’m not crazy about having a base class that makes this set of functionality universally available.

7

I certainly won’t be providing anything like this for vf::Listeners<>, especially because listeners can be added and removed by multiple threads concurrently.

8

Well, since Juce is now hiding implementation details in implementation pointers, if I need to change a small thing, either I have to rewrite the complete PImpl+component myself (with the changes, but it’s complete duplication of the code, and double the memory requirement of the library), either I deal with introspection and modify the stuff with it.
Currently, with introspection, it’s almost safe (provided I check the dyncast), I can change the structure, but I can’t change the behaviour, and this is painful.

Whatever the change you accept:

  • If you don’t want us to deal with listener pointers, we should be able to hook a listener (to prevent the initial listener from being called, replaced by ours). This sounds like a hack to me.
  • We could add return value to listeners (or a “abortPropagation” member to your ListenerCaller so our listener can stop propagating the event further down the chain), this would work, but you’d need to ensure listener call order (LIFC, last in, first called)
  • Let us call getListener(i), and if it happens while a listener chain is being called, return 0. There’s not point in changing the listeners while they are called, and it’s easy to do with a state variable.
  • Add a removeListeners() method (but I’m not fond of this method, since that means that we need to re-add any other listener that were added except the PImpl one)
  • Add introspection for all PImpl via a “void * getInternalImplementation()” in all component, so we can call “removeListener((Listener*)getInternalImplementation())”, but again, it’s ugly.

I prefer the second solution, as this is how it’s done in asynchronous programming languages.

9

  1. The juce::Slider is way too complicated and convoluted. I don’t see why it was necessary to use a Pimpl.

  2. If you need to do special cases or manipulations on a list of listeners, chances are that you’re using it wrong.

Goofing around with the listener list violates the basic contract of the observer pattern: that everyone gets called, and no high level code depends on who is listening.

10

[quote=“X-Ryl669”]For example, I’m using a +/- slider where I’m changing the “interval” step. However, my specific problem requires that the value are kept full resolution (so “inc step” with an interval of 10 applied to a value of 5 should result in 15, not 20).
I’ve subclassed the slider class, but due to overzealous Pimpl protection, I can’t change anything here without rewriting the whole stuff (which is counter productive).[/quote]

This is a problem of the Slider class having WAY too much responsibility. I talked about this in another thread, the Slider should be broken up into multiple, individual components each with their own class. A new Slider object should be created which is a composite Component containing these sub-objects, with appropriate hooks so that you can substitute your own subclasses for child objects. The functionality of the physical slider (horizontal, vertical, or rotary control that reacts to the mouse) should be isolated into one class that does just that, with the inc / dec buttons and edit text control in separate classes.

11

Ok, I agree for Slider, but you focus on a specific component, while I was discussing in general for all components.

I disagree with your remark about listener being an observer pattern. In Juce, even if it’s called “Listener”, it follows the signal/slot pattern in the idea, since you are not observing an event, you’re acting upon an event.
Being able to customize the action, by whatever mean is, IMHO, an absolute requirement. If you can’t do it without rewriting the complete class, then your design is wrong somehow.

That being said, listeners currently are just “copy and paste methods” in all component, and this exactly match for refactoring it. I think having a Listener base/manager/whatever class is a good idea, and I also think that we could just add a similar feature to all the Listener subclass (for example, have all the Listener subclass to follow a BaseListener interface with a “getName() = 0” for example).

That way, you’ll be able not to hack with listener’s by index in a transient listener list, but enumerate the listener you want to “disconnect” and provide your own.
The beauty of this, is that the logic of the application can be changed at runtime (thanks to the component introspection too).

12

Up.

I’ve grepped the code for addListener, and finally, the comments do not break the sky here. Basically, there are in the range “Register a listener that will be called when insert class name here changes”.
There is only 21 classes with addListener method.

So, I think the “special comment”-per-listener argument does not rely justify it, and I really need the functionnality to change the listener’s action at runtime.
About listener list updating while being called, I think it’s implementation details, (that can be explained in the comment that you can’t change the listener list while being in any listener’s callback, or check my code below for a solution)

Here’s my class attempt, if you want to tweak it:

/**
    When actions are required upon change, a common pattern is to register listeners with the class whose changes need monitoring. 
   This generalize the registering and unregistering of listeners, each manager is named so you can locate a specific manager if you need too
*/
template <class BaseClass>
class ListenerManager 
{
public:
    // Inject the internal Listener class inside us, so we can use it directly 
    typedef BaseClass::Listener Listener;
    
    /** Register a listener to be notified upon changes on the BaseClass */
    virtual void addListener (Listener* newListener); // See the macro below for an explanation for virtual here
    /** Unregister a listener to be notified upon changes on the BaseClass */
    virtual void removeListener (Listener * newListener);

    /** Get the listener manager name */
    virtual String getListenerManagerName() const = 0;

    /** Get the actual number of listeners in the list. 
         //optional This method can return 0 if called while the listeners are called to avoid manipulating the listener while it's running */
    int getListenersCount(); 
    /** Get the listener for the given index.
         // optional This method can return nullptr if called while the listeners are called to avoid manipulating the listeners while it's running */
    Listener * getListener(int index); 

private:
    // The actual listener list
    ListenerList <Listener> listeners;
};

// This is to replace any void addListener / removeListener code in each file, while keeping the specific comment for the file
#define JUCE_DECLARE_LISTENER(Class)  \
     void addListener(Class::Listener * listener) { ListenerBase<Class>::addListener(listener); }  \ // Added here so any comment above the macro here will also appear in the doxygen documentation in the base class
     String getListenerManagerName() const { return #Class ; }

My idea to avoid listener list manipulation while it could crash, is to add a bool per listener (in the ListenerList object), that’s initially false. If a listener is removed inside the caller’s iteration, then it’s not deleted directly, but this bool goes to “true”, and at the end of the iteration, it’s deleted. That way, it’s safe to remove a listener while it’s running.

13

Normally, I love anything that removes code duplication and which factors-out common functionality like this… but for some reason that I can’t explain, this just isn’t grabbing me! Wish I could put my finger on why I don’t like it, but it just doesn’t feel right for some reason…

14

Hmm, I kinda like it but also would like to see it at work to understand if it is actually helpful. What do you think about creating a feature branch with it so that other developers can work with that, too? If it appears to be helpful, you can merge it into the master at any point without expecting too many conflicts: the methods for adding/removing listeners have gone untouched for a while now

15

TBH I don’t have the spare brain-power to give it the attention it deserves right now.

16

I have the same thoughts.

Can you please explain your use-case for why you believe it is necessary for this feature to exist to make your code work? It breaks the Observer pattern and is just bad design in general. I feel that if you explain why you want such a thing, then we can find the flaw in the design and figure out the right way (which isn’t to break the Obsverer pattern contract).

And keep in mind that even if JUCE were to have such a feature, VFLib could never have the sort of thing that you’re asking for because it would require taking a critical section during a call and that would be unacceptable.

17

To be honest, Vincent, I don’t really care about vf::Listener in my request.
I think you’re misleading the discussion, since vf::Listener is not the point here. Please don’t advertize your work (which is excellent, TBH) in all threads of the forum, if it’s not completely related to the issue/improvement discussed, as it disturbs more than it helps.

Basically, Juce’s is more and more using hidden implementation for each object, which is good in terms of code maintainability for Jules, but not too much for us if we don’t want 100% what the code does or need to modify it.

Currently, it’s not possible to change the behaviour of the PImpl without rewriting the whole PImpl.
This is IMHO bad, since it means that the final software will have numerous duplicate of the code, with all the issues that it creates (if the PImpl code is bug-fixed after the initial fork, the forked code will likely have the initial issue, the binary will be bloated with 99.99% identic classes, the maintenance will be a burden, because for a very simple addition, you’ll have 100’s of lines of the original PImpl, etc…)

I think the design of the library is good, because it allow any class to figure out the component hierarchy, to modify this hierarchy from outside the component itself.
However, this initial design does not work well with listener & PImpl, since we can’t change what the PImpl’s are doing upon events, so we can’t tweak the behaviour.
If we were able to do so, then the whole thing would gain a HUGE maintainability improvement, since modifying some widgets behaviour does not require complete copy & paste of the PImpl anymore.

Basically, on Qt, you have signal & slot you can unconnect/reconnect the way you want to fit the behaviour your want, and you actually don’t care about the underlying implementation, as long as you’re doing the right stuff.
In Juce, you can’t do that NOW.

I want to be able to do “For this , I want to hook the action/event, modify it the way I want, and pass it to (or delete it before) the initial receiver(s)”.
For a slider class, I want to be able to hook the buttonClicked event so I can do some specific action, and I don’t want the Pimpl do what it does currently to actually change the value.
Or, I want to be able to change the Slider drag event so I can “tweak” the thumb position so it’s attracted like a magnetic fly on some specific keypoint value, without the PImpl doing the opposite.
I want to be able to hook the textChanged event of a texteditor to show autocomplete words
…etc…

To sum up, if I can say, “find out the PImpl listener, and don’t call it”, then I could work around most of my needs. Better karma point, if I can even do “call PImpl, then call me to fix the mess”, and/or the opposite.

18

Okay, now we get to the REAL problem. So…this is not best solved by breaking the rules of the observer pattern.

I agree that hiding the implementations for user interface controls is a terrible practice. I also think that the Slider is doing way too much.

But the answer is not to fool with listeners, it is to:

  1. Make juce::Slider no longer use a pimpl

  2. Break up the Slider code into five simpler interface items: up down buttons, thumb, groove, edit box, coordinator

  3. Allow newly created Slider objects to customize any or all of its constituent components

Jules already said he agreed that the Slider was getting too big. So instead of a hack which breaks the Observer pattern why don’t you instead make the changes I suggested about? You can make a new Slider based off the existing one and just give it a different name (like SliderGroup).

Jules would you be open to a new Slider which is flat (no pimpl) and is broken up into a couple of classes?

19

[quote=“X-Ryl669”]To be honest, Vincent, I don’t really care about vf::Listener in my request.
I think you’re misleading the discussion, since vf::Listener is not the point here. Please don’t advertize your work (which is excellent, TBH) in all threads of the forum, if it’s not completely related to the issue/improvement discussed, as it disturbs more than it helps.[/quote]

Of course its related, and I’m not advertising my work.

Since juce::ListenerList and vf::Listeners both implement the Observer pattern, they should follow the Liskov substitution principle. Your proposed change would break that. This is just another way of saying that you want to violate the Observer pattern contract. Both indicate bad design.

I will naturally be very vocal in my opposition to anything that changes the contract of the listener interface.

20

I’m sorry, but none of the idea I’ve proposed break the observer pattern. There is nothing in the observer pattern that prevent an observer from manipulating the observer’s list on the event sender (and in fact, if it was the case, it would be useless, since you usally modify the event sender to actually REACT on an event).
There is no subtype violation whatsoever (did you see any inheritance here?).
Currently all observers are equal (which would still work with my change), the order in which observers are called is not specified in the observer pattern (and I propose to agree on one, and be able to specify one programmatically).
I think you missed the other examples, concerning the TextEditor for example, and so on.
If I were to follow your idea, I would create hundred of classes by splitting all the widgets in Juces, and then it would be a nightmare to have a slider with a feature from “ButtonBaseSlider” and a feature from “LinearSlider”, like mouse dragging.

IMHO, if the slider class was to be split, it should be in different PImpl, all opaque to the user, AS LONG AS we can snoop into the hierarchy and modify it the way we want via the Component introspection interface.
What is mentally disturbing in allowing the behaviour to be changed at runtime ?