[Solved]Can't connect the signal to the value of the ProgressBar(would crash)



  • Using QtConcurrent::run to do some heavy work, this is the codes after simplify

    .hpp
    @
    #ifndef PROGRESSCONNECTION_HPP
    #define PROGRESSCONNECTION_HPP

    #include <QObject>

    class progressConnection : public QObject
    {
    Q_OBJECT
    Q_PROPERTY(int progressMaximum READ progressMaximum WRITE setProgressMaximum NOTIFY progressMaximumChanged)
    Q_PROPERTY(int progressValue READ progressValue WRITE setProgressValue NOTIFY progressValueChanged)
    public:
    explicit progressConnection(QObject *parent = 0);

    Q_INVOKABLE void addProgressValue();
    
    int progressMaximum() const;
    int progressValue() const;
    
    void setProgressMaximum(int value);
    void setProgressValue(int value);
    

    signals:
    void progressMaximumChanged();
    void progressValueChanged();

    private:
    void addProgressValueImpl();

    private:
    int progressMaximum_;
    int progressValue_;

    int size_;
    

    };

    #endif // PROGRESSCONNECTION_HPP

    @

    .cpp
    @
    #include <QtConcurrent/QtConcurrentRun>

    #include "progressConnection.hpp"

    progressConnection::progressConnection(QObject *parent) :
    QObject(parent),
    progressMaximum_(1),
    progressValue_(0),
    size_(100000)
    {
    }

    void progressConnection::addProgressValue()
    {
    setProgressMaximum(size_);
    setProgressValue(0);
    QtConcurrent::run(this, &progressConnection::addProgressValueImpl);
    }

    int progressConnection::progressMaximum() const
    {
    return progressMaximum_;
    }

    int progressConnection::progressValue() const
    {
    return progressValue_;
    }

    void progressConnection::setProgressMaximum(int value)
    {
    if(value != progressMaximum_){
    progressMaximum_ = value;
    qDebug()<<"progress maximum = "<<progressMaximum_;
    emit progressMaximumChanged();
    }
    }

    void progressConnection::setProgressValue(int value)
    {
    if(value != progressValue_){
    progressValue_ = value;
    qDebug()<<"progress value = "<<progressValue_;
    // emit progressValueChanged(); //if I emit this signal, the program may or may not crash
    }
    }

    void progressConnection::addProgressValueImpl()
    {
    //QMutexLocker lock(&mutex_);
    for(int i = 0; i != size_; ++i){
    setProgressValue(i);
    }
    }
    @

    main.cpp
    @
    #include <QApplication>
    #include <QQmlEngine>
    #include <QQuickView>
    #include <QtQml>

    #include "progressConnection.hpp"

    int main(int argc, char *argv[])
    {
    QApplication app(argc, argv);

    qmlRegisterType<progressConnection>("Test", 1, 0, "ProgressConnection");
    
    QQmlApplicationEngine engine(QUrl("qrc:/main.qml"));   
    QObject *topLevel = engine.rootObjects().value(0);
    QQuickWindow *window = qobject_cast<QQuickWindow *>(topLevel);
    if (!window) {
        qWarning("Error: Your root item has to be a Window.");
        return -1;
    }
    window->show();
    
    return app.exec&#40;&#41;;
    

    }

    @

    main.qml
    @
    import QtQuick 2.1
    import QtQuick.Controls 1.0
    import QtQuick.Dialogs 1.0
    import QtQuick.Layouts 1.0

    import Test 1.0

    ApplicationWindow{
    id: root

    title: "Color Correction"
    
    color: syspal.window
    width: 1450
    height: 750
    minimumHeight: 750
    minimumWidth: 1400
    
    SystemPalette {id: syspal}
    
    Component{
        id: compProgressTest
    
        ProgressConnection{
    
        }
    }
    
    Loader{
        id: loader
    
        sourceComponent: null
    
        onStatusChanged: {
            if(loader.status == Loader.Ready){
                console.log("Ready")
            }
        }
    }
    
    Row{
        anchors.fill: parent
    
        Button{
            text: "reload"
            onClicked: {
                loader.sourceComponent = null
                loader.sourceComponent = compProgressTest
            }
        }
    
        Button{
            text: "run"
            onClicked: {
                if(loader.status == Loader.Ready){
                    loader.item.addProgressValue()
                }
            }
        }
    
        ProgressBar{
            id: progressBar
    
            maximumValue: loader.status == Loader.Ready ? loader.item.progressMaximum : 1
            value: loader.status == Loader.Ready ? loader.item.progressValue : 0
    
            onValueChanged: {
                console.log("progress bar value = " + progressBar.value)
            }
        }
    }
    

    }
    @

    When I emit the signal of progressValueChanged(), the program would have some undefined behavior
    What kind of errors do I make?I could not emit a signal from another thread?


  • Moderators

    QObjects are not thread-safe, so you're not allowed to access a QObject from 2 different threads.

    Your ProgressConnection lives in the GUI thread, so setProgressValue() (and all the other member functions) must only be run in the GUI thread. Otherwise, 2 threads could try to read/write progressValue_ at the same time. That's probably what caused your crash.



  • Thanks, how about the emit signal?Could I emit the signal from different
    thread?

    Alter the codes a little bit, emit the signal directly
    @
    void progressConnection::addProgressValueImpl()
    {
    for(int i = 0; i != size_; ++i){
    setProgressValue(i);
    progressValue_ = i;
    qDebug()<<"progress value = "<<progressValue_;
    emit progressValueChanged();
    }
    }
    @

    Still got undefined behavior(crash, progress bar wouldn't update etc)
    I think qml also live at gui thread, and the value of ProgressBar is not thread safe
    I tried to add the mutex on the setProgressValue function, but it could not work either

    What are the correct solutions to change the value of the ProgressBar from another thread?


  • Moderators

    Your issue is not related to signals. You must not do this:
    @
    QtConcurrent::run(this, &progressConnection::addProgressValueImpl);
    @
    addProgressValueImpl() is a member function of ProgressConnection, so it must not run in a different thread.

    Your mutex didn't help you because it only stopped multiple threads from writing progressValue_ at the same time. Your program crashed possibly because the GUI was reading progressValue_ while the other thread was writing it through addProgressValueImpl()

    The correct way is:

    Declare ProgressConnect::setProgressValue() as a slot

    The other thread calculates the new value

    The other thread emits a signal that contains the new value

    Connect this signal to ProgressConnect::setProgressValue()

    The GUI thread runs ProgressConnect::setProgressValue() to update progressValue_

    ProgressConnect::setProgressValue() emits the progressValueChanged() signal from the GUI thread (this is already done automatically, since you set "NOTIFY" in Q_PROPERTY)

    The GUI thread updates the progress bar

    See the "Mandelbrot example":http://qt-project.org/doc/qt-4.8/threads-mandelbrot.html for another example on how a GUI can communicate with another thread for doing long calculations.



  • Hi, thanks for your helps, I try two solutions, the one from the example
    you gave me and the other one is the recommended way to use QThread

    solution 1( same as mandelbrot example) :

    ProgressWorkerThread2.hpp

    @
    #include <QThread>

    class QObject;

    class ProgressWorkerThread2 : public QThread
    {
    Q_OBJECT
    public:
    explicit ProgressWorkerThread2(int size, QObject *parent = nullptr);

    signals:
    void progressValueChanged(int);

    protected:
    void run();

    private:
    int size_;
    };
    @

    ProgressWorkerThread2.cpp
    @
    #include "ProgressWorkerThread2.hpp"

    ProgressWorkerThread2::ProgressWorkerThread2(int size, QObject *parent) :
    QThread(parent),
    size_(size)
    {
    }

    void ProgressWorkerThread2::run()
    {
    for(int i = 0; i != size_; ++i){
    emit progressValueChanged(i);
    }
    }

    @

    The change of the original codes
    .hpp
    @
    //turn the setProgressValue to slot
    public slots:
    void setProgressValue(int value);

    //.....
    private:
    ProgressWorkerThread2 *worker2_; //add new data member
    @

    .cpp
    @
    //initialize worker thread and connect the signal to slot
    progressConnection::progressConnection(QObject *parent) :
    QObject(parent),
    progressMaximum_(1),
    progressValue_(0),
    size_(100000),
    worker2_(new ProgressWorkerThread2(size_, this))
    {

    connect(worker2_, SIGNAL(progressValueChanged(int)), this, SLOT(setProgressValue(int)));
    

    }

    //call the worker thread
    void progressConnection::addProgressValue()
    {
    setProgressMaximum(size_);
    setProgressValue(0);

    worker2_->start();
    

    }
    @

    This solution crash



  • Solution 2(recommended way to use QThread)

    ProgressWorkerThread.hpp
    @
    #ifndef PROGRESSWORKERTHREAD_HPP
    #define PROGRESSWORKERTHREAD_HPP

    #include <QObject>

    class ProgressWorkerThread : public QObject
    {
    Q_OBJECT
    public:
    explicit ProgressWorkerThread(int size, QObject *parent = nullptr);

    public slots:
    void run();

    signals:
    void progressValueChanged(int);

    private:
    int size_;
    };

    #endif // PROGRESSWORKERTHREAD_HPP
    @

    ProgressWorkerThread.cpp
    @
    #include "ProgressWorkerThread.hpp"

    ProgressWorkerThread::ProgressWorkerThread(int size, QObject *parent) :
    QObject(parent),
    size_(size)
    {
    }

    void ProgressWorkerThread::run()
    {
    for(int i = 0; i != size_; ++i){
    emit progressValueChanged(i);
    }
    }
    @

    The change of the original codes
    .hpp
    @
    public slots:
    void setProgressValue(int value);

    private:
    QThread *thread_;
    ProgressWorkerThread *worker_;
    @

    .cpp
    @
    progressConnection::progressConnection(QObject *parent) :
    QObject(parent),
    progressMaximum_(1),
    progressValue_(0),
    size_(100000),
    thread_(new QThread(parent)),
    worker_(new ProgressWorkerThread(size_))
    {
    worker_->moveToThread(thread_);
    connect(worker_, SIGNAL(progressValueChanged(int)), this, SLOT(setProgressValue(int)));
    connect(thread_, SIGNAL(started()), worker_, SLOT(run()));
    }

    void progressConnection::addProgressValue()
    {
    setProgressMaximum(size_);
    setProgressValue(0);

    thread_->start();
    

    }
    @

    But this solution fail too

    InQt4, I use QtConcurrent::map and QFutureWatcher to update
    the progressBar value, but I can't do this in Qt5 due to some problem

    "map problem":qt-project.org/forums/viewthread/29759/



  • Both of the solutions handle the heavy jobs in another thread(call thread A)

    1 : run the heavy jobs in thread A
    2 : emit the signal from thread A
    3 : the slot connected by the worker are called and update the value
    of the ProgressBar from gui thread

    according to the document, the connection should queue the data
    from the signal automatically, and it should not have any data confliction
    What mistakes I commit this time?



  • Another experiment, this time I do not use any thread but update the
    value from the gui thread directly

    .hpp
    @
    #ifndef PROGRESSCONNECTION_HPP
    #define PROGRESSCONNECTION_HPP

    #include <QObject>

    class progressConnection : public QObject
    {
    Q_OBJECT
    Q_PROPERTY(int progressMaximum READ progressMaximum WRITE setProgressMaximum NOTIFY progressMaximumChanged)
    Q_PROPERTY(int progressValue READ progressValue WRITE setProgressValue NOTIFY progressValueChanged)
    public:
    explicit progressConnection(QObject *parent = 0);

    Q_INVOKABLE void addProgressValue();
    
    int progressMaximum() const;
    int progressValue() const;
    
    void setProgressMaximum(int value);
    void setProgressValue(int value);
    

    signals:
    void progressMaximumChanged();
    void progressValueChanged();

    private:
    void addProgressValueImpl();

    private:
    QMutex mutex_;

    int progressMaximum_;
    int progressValue_;
    
    int size_;
    

    };

    #endif // PROGRESSCONNECTION_HPP

    @

    .cpp
    @
    #include <QDebug>

    #include "progressConnection.hpp"

    progressConnection::progressConnection(QObject *parent) :
    QObject(parent),
    progressMaximum_(1),
    progressValue_(0),
    size_(100000)
    {
    }

    void progressConnection::addProgressValue()
    {
    setProgressMaximum(size_);
    setProgressValue(0);

    addProgressValueImpl();
    

    }

    int progressConnection::progressMaximum() const
    {
    return progressMaximum_;
    }

    int progressConnection::progressValue() const
    {
    return progressValue_;
    }

    void progressConnection::setProgressMaximum(int value)
    {
    if(value != progressMaximum_){
    progressMaximum_ = value;
    qDebug()<<"progress maximum = "<<progressMaximum_;
    emit progressMaximumChanged();
    }
    }

    void progressConnection::setProgressValue(int value)
    {
    if(value != progressValue_){
    progressValue_ = value;
    //qDebug()<<"progress value = "<<progressValue_;
    emit progressValueChanged();
    }
    }

    void progressConnection::addProgressValueImpl()
    {
    for(int i = 0; i != size_; ++i){
    setProgressValue(i);
    }
    }

    @

    The qml codes are same as the first post

    Amazingly, this would crash too



  • Give another try on Slider, the slider has no problem on single thread
    program.The multi-thread version also works fine

    @
    progressConnection::progressConnection(QObject *parent) :
    QObject(parent),
    progressMaximum_(1),
    progressValue_(0),
    size_(100000),
    thread_(new QThread(parent)),
    worker_(new ProgressWorkerThread(size_))
    {
    wworker_->moveToThread(thread_);
    connect(thread_, &QThread::finished, worker_, &QObject::deleteLater);
    //need Qt::BlockingQueuedConnection else the gui won't have time to update
    connect(worker_, SIGNAL(progressValueChanged(int)), this, SLOT(setProgressValue(int)), Qt::BlockingQueuedConnection);
    connect(this, SIGNAL(startProgress()), worker_, SLOT(run()));
    thread_->start();
    }

    void progressConnection::addProgressValue()
    {
    setProgressMaximum(size_);
    setProgressValue(0);

    emit startProgress();   
    

    }

    void progressConnection::setProgressValue(int value)
    {
    QMutexLocker lock(&mutex_); //I didn't expect I need this mutex
    if(value != progressValue_){
    progressValue_ = value;
    emit progressValueChanged();
    }
    }
    @

    Works fine too, Why Slider could work but ProgressBar can't?Am I doing anything wrong
    or this is a bug of ProgressBar?

    Why do we need a mutex?I thought after declaring Qt::BlockingQueuedConnection
    the current thread blocks until the slot returns so the currentThread should not reenter the function setProgressValue?

    os : mac 10.8.3
    compiler : clang3.2
    Qt version : 5.1


  • Moderators

    [quote author="stereomatching" date="1373451070"]Another experiment, this time I do not use any thread but update the
    value from the gui thread directly

    ...

    The qml codes are same as the first post

    Amazingly, this would crash too[/quote]Hmm... your experiment suggests that the error is somewhere else, not the threads. Both of your thread solutions look fine to me.

    Here are some more experiments you can try:

    • In one of your your thread solutions, slow down your signals (maybe 100000 signals in a short time is overwhelming your computer, especially since your console needs to print 100000 lines of text):
      @
      for(int i = 0; i != size_; ++i) {
      emit progressValueChanged(i);
      QThread::msleep(100);
      }
      @

    • Remove all your Loader code, and make ProgressBar.value a "Context Property":http://qt-project.org/doc/qt-5.1/qtqml/qtqml-cppintegration-contextproperties.html. This should make your QML code much cleaner too.



  • I try QThread::msleep(100) on multi-thread solution and single thread solution
    The value of the ProgressBar keep changing but the gui wouldn't update
    even I use QThread::sleep(10), the result remain the same

    It takes long time to run the whole test(running), so I am not sure
    the program would crash or not.Whatever, the gui wouldn't update when running.

    edit : looks like it would update, but the value is too large so I haven't noticed



  • The other experiment, I Remove all of my Loader codes(but do not make the ProgressBar as context property).

    I don't know why, after I replace the Loader codes with

    @
    ProgressConnection{
    id: progressConnection
    }
    @

    The program wouldn't crash again weather it is single thread or multi-thread(without sleep), pretty weird
    Do the loader of Qt5.1 has some bugs?Or my mistakes?


  • Moderators

    I haven't used Loaders before, so I don't really understand what's happening here. I just suggested removing it because it added timing complexity -- Simpler code is usually better ;)

    If you want, you can start a new thread (or post in the "Interest mailing list":http://lists.qt-project.org/mailman/listinfo/interest) to ask experienced QML programmers what's going on.

    But anyway, it looks like you've found a way to solve your original problem :)



  • Thanks, I would not know how to use the QThread without your helps.

    bq. Simpler code is usually better ;)

    I also like simpler code ;), that is why I select Loader.Sometimes
    the Loader could make code easier to maintain, but in this example
    it makes code more complicated, my fault, haven't made the codes
    simpler.

    My conditions

    @
    states:[
    State{
    name: ""
    PropertyChanges{ target: loaderPreview; sourceComponent: null}
    PropertyChanges{ target: loaderUI; sourceComponent: null}
    },
    State{
    name: Style.stateCLAHE

            PropertyChanges{ target: loaderPreview; sourceComponent: compCLAHEPreview}
            PropertyChanges{ target: loaderUI; sourceComponent: compCLAHEUI}
        },
        State{
            name: Style.stateGammaCorrect
    
            PropertyChanges{ target: loaderPreview; sourceComponent: compGammaCorrectPreview}
            PropertyChanges{ target: loaderUI; sourceComponent: compGammaCorrectUI}
        },
    

    ........
    @

    If I don't use loader, I have to add the PropertyChanges for every UI and preview in every states(plus other parameters)
    The codes would become something like
    @
    State{
    name: ""
    PropertyChanges{ target: compCLAHEPreview; visible: false}
    PropertyChanges{ target: compCLAHEUI; visible: false}
    PropertyChanges{ target: compGammaCorrectPreview; visible: false}
    PropertyChanges{ target: compGammaCorrectUI; visible: false}
    ........
    },
    State{
    name: Style.stateCLAHE
    PropertyChanges{ target: compCLAHEPreview; visible: true}
    PropertyChanges{ target: compCLAHEUI; visible: true}
    PropertyChanges{ target: compGammaCorrectPreview; visible: false}
    PropertyChanges{ target: compGammaCorrectUI; visible: false}
    }
    @

    Maybe there are easier solution to solve this problems, but I don't know how to
    If you have any ideas, please give me a hint ;), thanks


  • Moderators

    You're welcome :) Yeah, the "best" code architecture can be different for different purposes.

    I don't have any direct ideas for now, sorry. But keep learning and asking. Good luck!


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.