With this program I press the 'run' button and a for loop cycles 100 times (with a 100ms delay) and prints the cycle count in a txt field
I have successfully done it with a MyThread object derived from QThread. It works. I can interrupt the cycle with the 'stop' button.
However it is sternly warned that deriving an object from QThread is very bad. So I did it the other way they suggested, the "right" way.
And it doesn't work. I can get the loop cycle numbers out on the console but not into the text box
The 'run' button goes down and never comes up again until the 100 cycles are done. And the text field shows 99.
Here is the code, what am I doing wrong?
// MyWidget.h SF022
#ifndef MYWIDGET_H_
#define MYWIDGET_H_
#include <QtWidgets/QWidget>
#include <QtWidgets/QLineEdit>
#include <QtWidgets/QPushButton>
#include "../MyThread.h"
#include "../MyObject.h"
#include <QThread>
class MyWidget : public QWidget {
Q_OBJECT
public:
MyWidget(QWidget* parent = 0);
~MyWidget();
QPushButton* pbRun;
QPushButton* pbStop;
QPushButton* pbExit;
QLineEdit* txtCount;
QThread* cThread;
MyObject* myObject;
public slots:
void onNumberChanged(int);
private slots:
void pbRun_Slot();
void pbStop_Slot();
void pbExit_Slot();
};
#endif /* MYWIDGET_H_ */
------------------------------------------------
// MyWidget.cpp
#include "MyWidget.h"
#include "../K.h"
#include <iostream>
MyWidget::MyWidget(QWidget* parent) : QWidget(parent) {
pbRun = new QPushButton(this);
pbRun->setObjectName(QStringLiteral("pbRun"));
pbRun->setGeometry(QRect(20, 20, 80, 40));
pbRun->setText("Run");
connect(pbRun, SIGNAL(clicked()), this, SLOT(pbRun_Slot()));
pbStop = new QPushButton(this);
pbStop->setObjectName(QStringLiteral("pbStop"));
pbStop->setGeometry(QRect(20, 80, 80, 40));
pbStop->setText("Stop");
connect(pbStop, SIGNAL(clicked()), this, SLOT(pbStop_Slot()));
pbExit = new QPushButton(this);
pbExit->setObjectName(QStringLiteral("pbExit"));
pbExit->setGeometry(QRect(20, 140, 80, 40));
pbExit->setText("Exit");
connect(pbExit, SIGNAL(clicked()), this, SLOT(pbExit_Slot()));
txtCount = new QLineEdit(this);
txtCount->setGeometry(QRect(20, 200, 80, 40));
txtCount->setStyleSheet("QLineEdit{background: white;}");
// myObject holds the cycling mechanism
myObject = new MyObject(this);
// the myObject sends each new cycle number out here
connect(myObject, SIGNAL(numberChanged(int)), this, SLOT(onNumberChanged(int)));
}
MyWidget::~MyWidget() {
}
void MyWidget::pbRun_Slot() {
// start thread
cThread = new QThread(this);
myObject->doSetup(*cThread);
myObject->moveToThread(cThread);
cThread->start();
}
void MyWidget::pbStop_Slot() {
// stop the thread
myObject->Stop = true;
}
void MyWidget::pbExit_Slot() {
// a static pointer to the main window
(K::SfMainWin)->close();
}
// a slot
void MyWidget::onNumberChanged(int j) {
// output the cycle count to a text field
txtCount->setText(QString::number(j));
}
----------------------------------------------------------
// MyObject.h
#ifndef MYOBJECT_H_
#define MYOBJECT_H_
#include <QObject>
#include <QThread>
class MyObject : public QObject {
Q_OBJECT
public:
explicit MyObject(QObject* parent = 0);
~MyObject();
void doSetup(QThread&);
bool Stop;
signals:
void numberChanged(int);
public slots:
void doWork();
};
#endif /* MYOBJECT_H_ */
----------------------------------------------------------
// MyObject.cpp
#include "MyObject.h"
#include <QMutex>
#include <iostream>
#include "string.h"
MyObject::MyObject(QObject* parent) : QObject(parent) {
Stop = false;
}
MyObject::~MyObject() {
}
void MyObject::doSetup(QThread& cThread) {
Stop = false;
connect(&cThread, SIGNAL(started()), this, SLOT(doWork()));
}
void MyObject::doWork() {
for (int i = 0; i < 100; i++) {
QMutex mutex;
mutex.lock();
if (this->Stop) {
break;
}
// output into a text field
emit numberChanged(i);
// output on the console
std::cout << "running " << (QString::number(i)).toStdString() << std::endl;
mutex.unlock();
QThread::msleep(100);
}
}
myObject is never moved to the thread you created. Everything is being executed in the main thread. Because
myObject = new MyObject(this);
To move a QObject to another thread, he should not have a parent. If it does Qt will silently tell you something went wrong ( by printing on the output, same with incorrect connections). It is the framework design to not panic over this type of warnings...
It should have been
myObject = new MyObject(0);
Now that this is cleared, you have other defects in the code.
QMutex mutex; is local and will always be acquired by the same thread. which means he has no purpose. Instead it should be a private member of MyObjectMyWidget::pbStop_Slot should be a method of MyObject instead, otherwise you have a race condition when accessing Stop member. Remember the mutex above? It is the time to use it. By the way, with your implementation call directly the method, because the even loop of cThread is only executing doWork
MyObject::pbStop_Slot()
{
mutex.lock()
Stop = true;
mutex.unlock()
}
Now your program should be technically correct. But It suck that you cannot use signals and slots because your thread is blocked executing doWork. Also, can we do something about that lock. In fact, yes. The way I will go is to use a Qtimer as a heartbeat ech 100ms rather than asking the thread to sleep. But to not change much your code you can use QAbstractEventDispatcher * QAbstractEventDispatcher::instance ( QThread * thread = 0 ) directly
MyObject::pbStop_Slot() //becomes a real slot again
{
// no more mutex
Stop = true;
}
....
//this connection is changed
connect(pbStop, SIGNAL(clicked()), myObject, SLOT(pbStop_Slot()));
....
void MyObject::doWork() {
for (int i = 0; i < 100; i++) {
//no mutex
if (this->Stop) {
break;
}
// output into a text field
emit numberChanged(i);
// output on the console
std::cout << "running " << (QString::number(i)).toStdString() << std::endl;
//process events, to allow stop to be processed using signals and slots
QAbstractEventDispatcher::instance(cThread)->processEvents();
QThread::msleep(100);
}
}
A warning about processEvents. As it is now, if the user presses run while dowork is being executed it will be called within itself. You have a nasty code right now. An easy way to circumvent this is to put a boolean which is checked and set at the beginning of dowork.
dowork(){
if(isdoingwork)
return;
isdoingwork = true
for(...
This is a poor man way of achieving reentrancy . You will see the word reentrant quite often in Qt documentation.
Good luck in your multithreading trip.