Button setRepeatSpeed and setTriggeredOnMouseDown can cause infinite buttonclicked callbacks


#1

When debugging i noticed at some point my buttonClicked callback was being called repeatedly.
It turned out that my button on which i had set a repeatspeed was getting repeated over and over because the time it took to debug exceeded the repeat speed. I than added a sleep to simulate a lengthy operation (note that this happened also with a sleep for 400 ms) and i saw the same behaviour. The issue i was debugging was actually related, so although a button with a repeat speed wouldn't normally trigger such a lengthy operation this is actually a serious bug.

​
   button->setRepeatSpeed (300, 100, 20);
   button->setTriggeredOnMouseDown(true);

    void buttonClicked (Button* b) override

    {
        DBG("Clicked button");
        //simulate lengthy operation
        Thread::sleep(1000);
    }

 


#2

Obviously it's a cardinal sin to block the message thread for more than a few milliseconds, and would be particularly egregious to do so in a repeat callback like this one, so hopefully this would never be a problem in real code. But yes, point taken, it should probably have some safety checking to avoid getting into a loop. I'll look into it.


#3

Well you can't call debugging a 'cardinal sin' or can you.... ;-)


#4

No, of course not. But I actually can't reproduce a problem by adding a sleep - do you have a complete code snippet that shows it getting stuck?


#5

Well the code i used is a simple call to a method that takes a while to process, it's enumerating a certain kind of devices and that takes a while. You can of course set a breakpoint in the buttonClicked callback like i did and see the problem occur.
And just to be clear, don't you want to use the sleep or did you try the sleep and it nothing went wrong?


#6

I tried the sleep and it was fine. If you want me to look further, you'd need to give me a lump of code that definitely reproduces the problem, as I don't have time to experiment with it..


#7

I made the changes to your OSCMonitor example, i set the setTriggeredOnMouseDown and setRepeatSpeed to the clear button, in the clearButtonClicked i added a DBG and Thread::sleep, press the clear button and watch your output list clear over and over. Tested this on OSX El Capitan.

/*

  ==============================================================================


    This file was auto-generated!


  ==============================================================================

*/


#ifndef MAINCOMPONENT_H_INCLUDED

#define MAINCOMPONENT_H_INCLUDED


#include "../JuceLibraryCode/JuceHeader.h"

#include "OSCLogListBox.h"



//==============================================================================

class MainContentComponent   : public Component,

                               private Button::Listener,

                               private OSCReceiver::Listener<OSCReceiver::MessageLoopCallback>

{

public:


    //==========================================================================

    MainContentComponent()

        : portNumberLabel (new Label),

          portNumberField (new Label),

          connectButton (new TextButton ("Connect")),

          clearButton (new TextButton ("Clear")),

          connectionStatusLabel (new Label),

          oscLogListBox (new OSCLogListBox),

          oscReceiver (new OSCReceiver),

          currentPortNumber (-1)

    {

        setSize (700, 400);


        portNumberLabel->setText ("UDP Port Number: ", dontSendNotification);

        portNumberLabel->setBounds (10, 18, 130, 25);

        addAndMakeVisible (portNumberLabel);


        portNumberField->setText ("9001", dontSendNotification);

        portNumberField->setEditable (true, true, true);

        portNumberField->setBounds (140, 18, 50, 25);

        addAndMakeVisible (portNumberField);


        connectButton->setBounds (210, 18, 100, 25);

        addAndMakeVisible (connectButton);

        connectButton->addListener (this);


        clearButton->setBounds (320, 18, 60, 25);

        addAndMakeVisible (clearButton);

        clearButton->addListener (this);

        clearButton->setTriggeredOnMouseDown(true);

        clearButton->setRepeatSpeed(300, 100, 20);

        


        connectionStatusLabel->setBounds (450, 18, 240, 25);

        updateConnectionStatusLabel();

        addAndMakeVisible (connectionStatusLabel);


        oscLogListBox->setBounds (0, 60, 700, 340);

        addAndMakeVisible (oscLogListBox);


        oscReceiver->addListener (this);

        oscReceiver->registerFormatErrorHandler (

            [this] (const char* data, int dataSize)

            {

                oscLogListBox->addInvalidOSCPacket (data, dataSize);

            }

        );

    }


private:

    //==========================================================================

    ScopedPointer<Label> portNumberLabel;

    ScopedPointer<Label> portNumberField;

    ScopedPointer<TextButton> connectButton;

    ScopedPointer<TextButton> clearButton;

    ScopedPointer<Label> connectionStatusLabel;


    ScopedPointer<OSCLogListBox> oscLogListBox;

    ScopedPointer<OSCReceiver> oscReceiver;


    int currentPortNumber;



    //==========================================================================

    void buttonClicked (Button* b) override

    {

        if (b == connectButton)

            connectButtonClicked();

        else if (b == clearButton)

            clearButtonClicked();

    }


    //==========================================================================

    void connectButtonClicked()

    {

        if (! isConnected())

            connect();

        else

            disconnect();


        updateConnectionStatusLabel();

    }


    //==========================================================================

    void clearButtonClicked()

    {

        oscLogListBox->clear();

        DBG("clear");

        Thread::sleep(1000);

    }


    //==========================================================================

    void oscMessageReceived (const OSCMessage& message) override

    {

        oscLogListBox->addOSCMessage (message);

    }


    void oscBundleReceived (const OSCBundle& bundle) override

    {

        oscLogListBox->addOSCBundle (bundle);

    }


    //==========================================================================

    void connect()

    {

        int portToConnect = portNumberField->getText().getIntValue();


        if (! isValidOscPort (portToConnect))

        {

            handleInvalidPortNumberEntered();

            return;

        }


        if (oscReceiver->connect (portToConnect))

        {

            currentPortNumber = portToConnect;

            connectButton->setButtonText ("Disconnect");

        }

        else

        {

            handleConnectError (portToConnect);

        }

    }


    //==========================================================================

    void disconnect()

    {

        if (oscReceiver->disconnect())

        {

            currentPortNumber = -1;

            connectButton->setButtonText ("Connect");

        }

        else

        {

            handleDisconnectError();

        }

    }


    //==========================================================================

    void handleConnectError (int failedPort)

    {

        AlertWindow::showMessageBoxAsync (

            AlertWindow::WarningIcon,

            "OSC Connection error",

            "Error: could not connect to port " + String (failedPort),

            "OK");

    }


    //==========================================================================

    void handleDisconnectError()

    {

        AlertWindow::showMessageBoxAsync (

            AlertWindow::WarningIcon,

            "Unknown error",

            "An unknown error occured while trying to disconnect from UPD port.",

            "OK");

    }


    //==========================================================================

    void handleInvalidPortNumberEntered()

    {

        AlertWindow::showMessageBoxAsync (

            AlertWindow::WarningIcon,

            "Invalid port number",

            "Error: you have entered an invalid UDP port number.",

            "OK");

    }


    //==========================================================================

    bool isConnected()

    {

        return currentPortNumber != -1;

    }


    //==========================================================================

    bool isValidOscPort (int port)

    {

        return port > 0 && port < 65536;

    }


    //==========================================================================

    void updateConnectionStatusLabel()

    {

        String text = "Status: ";

        if (isConnected())

            text += "Connected to UDP port " + String (currentPortNumber);

        else

            text += "Disconnected";


        Colour textColour = isConnected() ? Colours::green : Colours::red;


        connectionStatusLabel->setText (text, dontSendNotification);

        connectionStatusLabel->setFont (Font (15.00f, Font::bold));

        connectionStatusLabel->setColour (Label::textColourId, textColour);

        connectionStatusLabel->setJustificationType (Justification::centredRight);

    }


    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MainContentComponent)

};



#endif  // MAINCOMPONENT_H_INCLUDED

 


#8

OK, I've added a workaround that will stop this blocking forever.

But it's still the case that you should never block the GUI thread for long periods of time! Even 50ms is bad if you do it repeatedly, and 1sec is really terrible behaviour under any circumstances.


#9

Cool, and remember i'm trying to help you here, i posted this issue because i ran into this while debugging. I'm just trying to make sure nobody runs into the same issue.