Deprecating method based listener callbacks

// There are now lambda-based call functions that can be used to replace these old method-based versions.
// We'll eventually deprecate these old ones, so please begin moving your code to use lambdas!

Guys - don’t deprecate these. Just leave them there as an alternative. We have 1000s of these things and they can coexist at very low cost!

7 Likes

I agree 100%

It is not only the amount of legacy code, listeners have advantages in some cases:

  • You can add as many listeners without wiping out the previous callback
  • Sometimes you have a listener class that you use on different places in the codebase, where you would have to copy around the lambda code or create lambda factories

Just leave them where they are, please

3 Likes

I don’t think this is talking about deprecating the Listener classes in JUCE, it’s about deprecating the old methods in ListenerList that take function pointers, rather than a lambda.

I can see why you wouldn’t want to take on unnecessary work to change all your listener calls… but I don’t see any issue with deprecating these old ones to make it clear they’re no longer recommended and any new code should use the lambda approach.

Deprecation doesn’t mean they’ll be removed, just won’t be supported any longer.

1 Like

The comment says: “please begin moving your code to use lambdas!”, not “we suggest you use the newer lambda methods”.

I’ve always been a fan of the function pointer call methods it means I can write something like…

m_listeners.call (&Listener::onSomethingChanged, *this);

instead of…

m_listeners.call ([this] (Listener& listener) { listener. onSomethingChanged (*this); });

In this simple case I’ve always felt the function pointer communicates intent much clearer than the lambda :man_shrugging:.

6 Likes

Yeah, I think the listener call method looks perfectly clean without the lambda. What’s the advantage of the lambda?

I think you should be able to make something up with std::bind. Haven’t tried myself yet. I think the std::bind approach should also enable you to pass the function parameters to your listener function.

Beat me to it.

Yeah, you could do the exact same thing with std::bind if you really wanted to have the callback call a member function.

Which I’m tempted say is even clearer than the original example since it reads as “bind my member function to this callback”.

Here is what I just came up with:

    class Listener
    {
      public:
        virtual ~Listener() noexcept = default;
        virtual void anyActiveChanged(bool value);
    };

listeners.call(std::bind(&Listener::anyActiveChanged, std::placeholders::_1, true));

You might want to declare a global const for your first placeholder and name it more appropriately, like Placeholder::listener or something like this.

My point is that JUCE should aim for backwards compatibility in so far as is practical. Especially when the backwards compatibility doesn’t prevent the addition of better ways of doing things.

To be clear it’s a very personal thing and I’m happy with whatever JUCE decides is right for the library, backwards compatibility is great but it almost always comes at a cost. I realise that there is a cost on both ends too, in the library, and in the projects using the library.

I personally don’t think std::bind communicates the intent clearer because…

  1. IMO the function pointer method reads better. It reads closer to what I actually want.
  2. The point is that you are passing an unbound member function pointer that will later be bound to each listener instance, hence why you must insert the placeholder when doing the std::bind.

As always there are multiple solutions to the problem, templates could potentially be used rather than an argument. I also suspect we could make a generic template function that works for all cases, and that function could be made very clear with the use of C++20 concepts.

Another option is to take the abstraction a little further and follow the style being used in the library in several other places with a public lambda, but go a step further and implement something akin to signals and slots.

In general though I think C++ could make several improvements to allow functions and member functions to be passed around like other function objects rather than having to create lambdas or pass them as pointers. For example the only thing I don’t like about the syntax of the function pointer is that I have to pass it as a pointer and therefore add the ampersand to get the address. IMO it would be sensible if I didn’t have to add the ampersand.

I had completely forgotten about std::mem_fn. This would remove the need for the placeholder and it allows pointers, references, and smart pointers to be passed as the instance argument which should make it compatible with the new API. This means we could write…

m_listeners.call (std::mem_fn (&Listener::onSomethingChanged), *this);

I guess if the functions were removed this seems like the most painless change to make.

1 Like

This function is also marked to be deprecated in the future.

Yeah I realised that actually std::mem_fn still doesn’t help that much because ultimately I would still prefer to pass the parameters to the call function, and the proposed replacements only take a callable with one argument (a reference to a listener). Even doing that with std::bind isn’t great as I think it will do copies where you will want references or perfect forwarding. So I still come back to the function pointer having the clearest intent. i.e. please call this function with these parameters on all my listeners.

I don‘t think bind does any copying or moving. Its most certainly passed with std::forward and if you‘re handling with templates yourself, you can do so too. Everything else should be removed by the compiler using copy elision.

Unfortunately IME it does copy when you might not expect it to so you have to wrap arguments with std::ref for example.

1 Like

I also like the function pointer overload and we use it as default choice when dealing with juce Listener implementations all over our codebase.

The only thing that I dislike is actually that C++ is inconsistent in its requirements to add an ampersand before non static member functions in contrast to static ones and free functions in order to assign them to a function pointer, but I might be overlooking something. Maybe in a future version of C++ this requirement will be dropped and we can write listeners.call (Listener::foo, bar) which would be beautiful code to me.

However, the comment cited above is four years old and no deprecation has been added during that time (while other API changes have happened during that time). Before discussing a lot of theoretical scenarios and discussing appropriate replacement approaches in this theoretical scenarios, maybe the JUCE devs (maybe @reuk ?) would like to give a short statement

  • If deprecation of these functions is still on their backlog?
  • In case it is, why? I assume there was a good reason for that comment four years ago
  • In case it is not, if they want to remove that comment?
1 Like

Given that ListenerList is not too complex and doesn’t require much maintenance, I think the main argument for removing the old functions would be to avoid duplication in the API. When there are multiple ways of doing the same thing, users start to wonder whether there are trade-offs between the different approaches, and it can also lead to inconsistent-looking code.

However, I (personally) don’t feel that this is a strong enough reason to force users to update/modify code that currently works. For that reason, I’d be in favour of removing the warning comment, in this case.

That said, there are plenty of other deprecation warnings in JUCE that we do intend to follow through on, so please continue to avoid deprecated functions wherever possible!

7 Likes

Thanks :slight_smile:

My half a million lines of code have just relaxed …