ButtonParameterAttachment and button notification bug

Currently, if you use a ButtonParameterAttachment and automate the attached parameter, ButtonListener::buttonClicked() and Button::onClick are called.

They should not be called, only buttonStateChanged should be called instead.

any chance this could be addressed?

I think this is the intended behaviour and there are probably a lot of applications that rely on the behaviour being that way.

You can relatively easy create your own attachment that does not trigger a button click.

But I think you having this problem is indicating that the button is doing more than one thing? In my opinion that is either not good UI design or if you need the click listeners you may need to rethink your software design to get into the realm of how JUCE is intended to be used. But without any context I’m just guessing here. Maybe if you share what it is you are trying to achieve, we can assist with getting around your problem.

Thanks for your reply.
I don’t think it’s the intended behaviour to broadcast a ‘click’ when there hasn’t been one?

I can’t easily create my own attachment that would only trigger a ‘state change’, because the following function is private:
void Button::setToggleState (bool shouldBeOn, NotificationType clickNotification, NotificationType stateNotification);

(there’s a request about that btw: Expose private method setToggleState(bool, clickNotification, stateNotification) from Button )

Maybe if you share what it is you are trying to achieve

well, I’m just trying to know when the user clicks on the button to trigger a specific action.
I don’t want that to happen when the change is coming from an automation.

Well, I don’t think that is possible as it is right now. You could maintain your own version of JUCE and just make that function public.

I know I can maintain my own version of juce yes, but I would rather like the bug to be fixed or the feature request to be fulfilled. I would not have started this thread otherwise :slight_smile:

You can see that apart from get/setToggleState, there’s also get/setState. State and toggle state are different things. State can be normal, over or down, as listed in the ButtonState enum. It relates only to mouse interaction. How this is notified to listeners is a bit confusing. buttonStateChanged is called when either the state or the toggle state change. buttonClicked is called when there’s an actual click, with or without toggle, and also when there’s a toggle without click, unless it comes from a connected Value object. As confusing as it may be, too many things depend on this behavior -for example parameter attachments, which need to react to toggles but not to (mouse) state changes, and hence use buttonClicked.

1 Like

Can’t add much to that! The fact that the click pipeline is triggered when state changes is very convenient in a lot of places (e.g. having a toggle button, attaching an onClick callback to get notified when the user changes the state and this callback being triggered when #setToggleState is switched from anywhere else but the user).

In this case onClick is of course poorly named since onClick isn’t really what is happening here. As I wrote above, the intended workflow would call for a name like onStageChanged but that is not possible due to the abstraction in the class juce::Button.

So looking at the concrete behaviour, that the juce::ToggleButton triggers the onClick callback when the state is changed even though the user didn’t click is of course very counter intuitive, but makes a lot of sense when you look at the bigger picture.

+1 from me, I am struggling to find a workaround for a while.

A fork is no solution for me as well, because I use it in PluginGuiMagic, which is a module to be used by other juce users, and they might not be able to use my fork, because maybe they rely on their own fork.

There must be a way to change the state of a toggle button without sending a click. For me it leads into feedback circles when a parameter attachment changes a button that is part of a button group.

Convenience cannot be an argument here. If so, it should somehow be an opt-in feature. You can get the convenient behaviour by listening to the state change instead. But there is no way to opt out.

If you mean the current buttonStateChanged, you can’t: it also notifies when, for example, the mouse hovers the button. Probably the right thing would be to have clicked/onClick for actual clicks, stateChanged only for normal/over/down, and separate toggleStateChanged/onToggleStateChange. It would break a lot of code though.

Well, at least there is a workaround: just keep the old clicked state as member and whenever stateChanged occurs, you can compare to the previous state?

While on the other side there is no workaround to change the clicked state without sending a wrong clicked signal.

It’s possible, but it would be a very messy workaround that would affect a lot of existing code, practically forcing to pair all toggle buttons with a bool, while still having to change all callbacks -a breaking change not any better than adding a new callback altogether. Even if the naming is misleading, there’s some kind of consistency: buttonClicked works like comboBoxChanged or sliderValueChanged in that none of them distinguish actual UI interaction from any other source, as if it was considered an internal detail. That may be particularly bad for buttons, but it still seems deliberate.

So do you have a suggestion to solve the issue, how to change the toggle state programmatically without sending a bogus onClick event?

No, and I actually had other problems with juce::Button and ended up making my own button class. I was just pointing that buttonStateChanged doesn’t do what it may seem, and solving this with proper naming needs a pretty invasive breaking change.

Ok, thanks for your input anyway.

Maybe somebody from the juce team can then chime in and come up with a workaround or a fix please.

thank you for pointing this out.
when changing the Value object directly only a stateNotification is broadcasted, but no clickNotification. So the workaround we were all looking for is just to do:
button.getToggleStateValue() = shouldBeOn;
instead of using setToggleState()

Regarding the parameter attachment behaviour I was looking for, I can now write my own one like this:

class CustomButtonParameterAttachment   : private Button::Listener
{
public:
    CustomButtonParameterAttachment (RangedAudioParameter& param, Button& b,
                                     UndoManager* um = nullptr)
    : button (b)
    , attachment (param, [this] (float f) { setValue (f); }, um)
    {
        sendInitialUpdate();
        button.addListener (this);
    }

    ~CustomButtonParameterAttachment() override
    {
        button.removeListener (this);
    }

    void sendInitialUpdate()
    {
        attachment.sendInitialUpdate();
    }

private:
    void setValue (float newValue)
    {
        button.getToggleStateValue() = (newValue >= 0.5f);
    }

    void buttonClicked (Button*) override
    {
        attachment.setValueAsCompleteGesture (button.getToggleState() ? 1.0f : 0.0f);
    }

    Button& button;
    ParameterAttachment attachment;
};