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

[stereomatching]

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?

[JKSH]

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.

[stereomatching]

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?

[JKSH]

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.

[stereomatching]

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

[stereomatching]

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/

[stereomatching]

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?

[stereomatching]

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

[stereomatching]

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

[JKSH]

[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.

[stereomatching]

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

[stereomatching]

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?

[JKSH]

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 :)

[stereomatching]

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

[JKSH]

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!