1

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

    }
}
Sequoia
  • 33
  • 5
  • Have a look at the following link for some insight: https://mayaposch.wordpress.com/2011/11/01/how-to-really-truly-use-qthreads-the-full-explanation/ – Tyler Jandreau Jan 26 '15 at 20:23
  • If this is just an exercise, then OK, but usually you'd do stuff like this with a `QTimer` and no worker threads. Read [this](http://qt-project.org/wiki/Threads_Events_QObjects#c2e33c61ced29e1eb1bee9a1671f202e) for more information about it. – thuga Jan 27 '15 at 08:39

2 Answers2

1

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.

  1. 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 MyObject

  2. MyWidget::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()
}

  3. 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);    
}

    }

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

Community
  • 1
  • 1
UmNyobe
  • 22,539
  • 9
  • 61
  • 90
0

Very good UmNyobe!

For reference I've added the corrected code here.

// MyWidget.h   

#ifndef MYWIDGET_H_
#define MYWIDGET_H_

#include <QtWidgets/QWidget>
#include <QtWidgets/QLineEdit>
#include <QtWidgets/QPushButton>

#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* wThread;

    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");

    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 = new MyObject(0);

    connect(myObject, SIGNAL(numberChanged(int)), this, SLOT(onNumberChanged(int)));

    connect(pbStop, SIGNAL(clicked()), myObject, SLOT(pbStop_Slot()));
}

MyWidget::~MyWidget() {

    delete myObject;
    delete wThread;
}

void MyWidget::pbRun_Slot() {

//  start QThread*, wThread in the MyWidget class

    wThread = new QThread(this);
    myObject->doSetup(wThread);
    myObject->moveToThread(wThread);
    wThread->start();
}

void MyWidget::pbExit_Slot() {

//  a static pointer of the main window

    (K::SfMainWin)->close();
}

void MyWidget::onNumberChanged(int j) {

    txtCount->setText(QString::number(j));
}
---------------------------------------------------------
// MyObject.h

#ifndef MYOBJECT_H_
#define MYOBJECT_H_

#include <QObject>
#include <QThread>
#include <QMutex>

class MyObject : public QObject {

    Q_OBJECT

public:
    explicit MyObject(QObject* parent = 0);
    ~MyObject();

    void doSetup(QThread*);

    int  hold;
    bool Stop;
    int  inx;
        int  lastUsedInx;

signals:
    void numberChanged(int);

public slots:
    void doWork();
    void pbStop_Slot();

private:
    bool      isdoingwork;
    QThread*  pThread;
    QMutex    mutex;
};

#endif /* MYOBJECT_H_ */
----------------------------------------------------
// MyObject.cpp    SF022

#include "MyObject.h"

#include <iostream>

#include <QAbstractEventDispatcher>

MyObject::MyObject(QObject* parent) : QObject(parent) {

    Stop = false;
    isdoingwork = false;
    inx = 0;
    lastUsedInx = 0;
}

MyObject::~MyObject() {

}

void MyObject::doSetup(QThread* thread) {

    pThread = thread;
    Stop = false;
    isdoingwork = false;
    connect(pThread, SIGNAL(started()), this, SLOT(doWork()));
}

void MyObject::pbStop_Slot() {

    mutex.lock();
    Stop = true;
    isdoingwork = false;
    mutex.unlock();
}

void MyObject::doWork() {

    if(isdoingwork) {

        return;
    }

    isdoingwork = true;

    for (inx = lastUsedInx + 1; inx < 100; inx++) {

        if (this->Stop) {

            break;
        }

//  output into a text box
        emit numberChanged(inx);

        lastUsedInx = inx;

//  process events, to allow stop to be processed using signals and slots
        (QAbstractEventDispatcher::instance(pThread))->processEvents(QEventLoop::AllEvents);

        QThread::msleep(800);
    }

    isdoingwork = false;
}
Sequoia
  • 33
  • 5