QThread misunderstanding...I suppose!

I am not new in Posix threads but new in QThread. The expected behaviour of my code is both threads increasing a shared variable named val. Unfortunately it appears that each one is working on a different instance of val, ie if I clic twice on button 1 I get val=2 and then if I clic on button 2 I get val=0.
What is wrong in my code?
Thanks in advance for your help.

Code:

---

class boutonThread : public QThread
{
  Q_OBJECT
 
public:
  boutonThread(QWidget *parent = 0, int number = 0);
 
public slots:
  void threadClicked();
 
private:
  QMessageBox *msg;
  int val;
  QVector<long> thread_ids;
  int GetId();
};
 
class boutons : public QWidget
{
  Q_OBJECT
 
public:
  boutons(QWidget *parent = 0);
 
private:
  QPushButton *buttons[2];
  buttonThread *threads[2];
};
 
boutons::boutons(QWidget *parent)
{
  QHBoxLayout *layout = new QHBoxLayout(this);
  buttons[0] = new QPushButton("Thread 1", this);
  layout->addWidget(buttons[0]);
  threads[0] = new boutonThread(this, 1);
  QObject::connect(this->buttons[0], SIGNAL(clicked()), threads[0], SLOT(threadClicked()));
  buttons[1] = new QPushButton("Thread 2", this);
  layout->addWidget(buttons[1]);
  threads[1] = new boutonThread(this, 2);
  QObject::connect(this->buttons[1], SIGNAL(clicked()), threads[1], SLOT(threadClicked()));
  threads[0]->start();
  threads[1]->start();
}
 
boutonThread::boutonThread(QWidget *parent, int number)
{
  this->thread_ids.resize(2);
  this->thread_ids[number-1] = (long)QThread::currentThread();
  this->val = 0;
}
 
int
boutonThread::GetId()
{
  return this->thread_ids.indexOf((long)QThread::currentThread()) + 1;
}
 
void
boutonThread::threadClicked()
{
  QMessageBox *msg = new QMessageBox(QMessageBox::NoIcon, QString(), QString("Thread %1 : val = %2").arg(this->GetId()).arg(this->val), QMessageBox::Ok);
  this->val++;
  msg->exec();
}

---

Define val as static. And of course :
1. Don't init value in class constructor.
2. Use some method of synchronizing access to val

Apparently you are not familiar with C++ and Object-Oriented Programing.
You have made so many mistakes that I don't know how to start to fix it (Lesiok didn't even scratch the surface of the problem).

Maybe it will be better if you explain what are you trying to do.

Quote:

---

Originally Posted by MarekR22

Apparently you are not familiar with C++ and Object-Oriented Programing.
You have made so many mistakes that I don't know how to start.

---

Ok man so describes these "so many mistakes", I am really curious about it.

---

Added after 7 minutes:

Quote:

---

Originally Posted by Lesiok

Define val as static. And of course :
1. Don't init value in class constructor.
2. Use some method of synchronizing access to val

---

You mean declaring val outside of the QThread subclass declaration?
In the examples given in the doc - mandelbrot and fortune client - there is no such declaration and I suppose that it works despite it, doesn't it?
2. Right but there is no concurrent access at the same time, due to button based triggering, and anyway this program is just for testing purpose.

1. sublclassing QThread. In general it is not needed but thing that you did is useless, you didn't override run(), this slot will not be invoked in this thread; simply you don't have mutithreading.
2. variable 'val' in strange place it should be in some other object
3. QVector<long> thread_ids; - why here? what are you doing?
4. (long)QThread::currentThread() - very nasty thing why you need that?

so in general you have such mess that I'm unable to fix, mainly because I don't known what are you trying to achieve.

Quote:

---

Originally Posted by Caius Aérobus

Ok man so describes these "so many mistakes", I am really curious about it.

---

I'd begin with declaring a class member variable and expecting it to be shared by multiple instances of that class.

Since you say you are familiar with POSIX threads, I'm also surprised that you didn't do anything to synchronize access to the shared variable. Not that your threads are doing anything, as Marek has already explained...

Your calls to currentThread() also don't make any sense -- you can easily verify they all return the same value regardless where and how you call them.

Ok so you just did not understand what I want to do and conclude that I am not familiar with C++, funny conclusion!

Quote:

---

Originally Posted by MarekR22

1. sublclassing QThread. In general it is not needed but thing that you did is useless, you didn't override run(), this slot will not be invoked in this thread; simply you don't have mutithreading.

---

It is just that the default QThread::run() method is enough for my needs, is it wrong? I just want to have 2 threads runnning and reacting to signals so I define a method to be attached to a signal and I do not have anything special to code in run(), the code is in the slot. May be not the best way to do it but it seems that this works actually.

Quote:

---

Originally Posted by MarekR22

1. variable 'val' in strange place it should be in some other object

---

This is a variable expected to be shared by all the threads, I believed that all instances of a class inherited by QThread share common variables but actually I did not find any explanation on the way Qt has made things with Threads so I have to imagine how it works actually.

Quote:

---

Originally Posted by MarekR22

1. QVector<long> thread_ids; - why here? what are you doing?

---

Probably but I did not find a better way of identifying threads so as to determine which one is number 1 and which one is number 2.

Quote:

---

Originally Posted by MarekR22

1. (long)QThread::currentThread() - very nasty thing why you need that?

so in general you have such mess that I'm unable to fix that, mainly because I don't known what are you trying to achieve.

---

Quote:

---

Originally Posted by Caius Aérobus

It is just that the default QThread::run() method is enough for my needs, is it wrong? I just want to have 2 threads runnning and reacting to signals so I define a method to be attached to a signal and I do not have anything special to code in run(), the code is in the slot. May be not the best way to do it but it seems that this works actually.

---

No, it doesn't work. Your QThread object lives in the main thread so all its slots are executed in context of the main thread.



This is a variable expected to be shared by all the threads, I believed that all instances of a class inherited by QThread share common variables but actually I did not find any explanation on the way Qt has made things with Threads so I have to imagine how it works actually.

Probably but I did not find a better way of identifying threads so as to determine which one is number 1 and which one is number 2.[/QUOTE]

[QUOTE=wysota;242054]No, it doesn't work. Your QThread object lives in the main thread so all its slots are executed in context of the main thread.

But this way the "val" variable should be modified in the context of the main thread hence it should increase by 1 each time the slot is executed, isn't it?
BTW I have defined run() in my buttonThread class, just putting exec() in it, and got the same result.

The problem you are facing is not due to QThread, it basically ia a C++ object concept problem. I tried to modify your code at a minimum to get to work the way you wanted.

BTW, I should say that your OP has nothing to with QThread (As already explained by others). You just defined them and off-course started them, but they just don't do anything. May be you were just doing some experiment.

Code:

---

#include <QtGui>
#include <QApplication>
 
 
class buttonThread : public QThread
{
    Q_OBJECT
 
public:
    buttonThread(QWidget *parent = 0, int number = 0);
 
public slots:
    void threadClicked();
 
private:
    QMessageBox *msg;
    static int val;
    QVector<long> thread_ids;
    int GetId();
};
 
int buttonThread::val = 0;
 
class boutons : public QWidget
{
    Q_OBJECT
 
public:
    boutons(QWidget *parent = 0);
 
private:
    QPushButton *buttons[2];
    buttonThread *threads[2];
};
 
boutons::boutons(QWidget *parent)
{
    QHBoxLayout *layout = new QHBoxLayout(this);
    buttons[0] = new QPushButton("Thread 1", this);
    layout->addWidget(buttons[0]);
    threads[0] = new buttonThread(this, 1);
    QObject::connect(this->buttons[0], SIGNAL(clicked()), threads[0], SLOT(threadClicked()));
    buttons[1] = new QPushButton("Thread 2", this);
    layout->addWidget(buttons[1]);
    threads[1] = new buttonThread(this, 2);
    QObject::connect(this->buttons[1], SIGNAL(clicked()), threads[1], SLOT(threadClicked()));
    threads[0]->start();
    threads[1]->start();
}
 
buttonThread::buttonThread(QWidget *parent, int number)
{
    this->thread_ids.resize(2);
    this->thread_ids[number-1] = (long)QThread::currentThread();
}
 
int
buttonThread::GetId()
{
    return this->thread_ids.indexOf((long)QThread::currentThread()) + 1;
}
 
void
buttonThread::threadClicked()
{
    QMessageBox *msg = new QMessageBox(QMessageBox::NoIcon, QString(), QString("Thread %1 : val = %2").arg(this->GetId()).arg(val), QMessageBox::Ok);
    val++;
    msg->exec();
}
 
int main(int argc, char *argv[])
{
    QApplication app(argc, argv);
    boutons w;
 
    w.show();
    return app.exec();
}
 
#include "main.moc"

---

Quote:

---

Originally Posted by Santosh Reddy

The problem you are facing is not due to QThread, it basically ia a C++ object concept problem. I tried to modify your code at a minimum to get to work the way you wanted.

BTW, I should say that your OP has nothing to with QThread (As already explained by others). You just defined them and off-course started them, but they just don't do anything. May be you were just doing some experiment.

---

What you did is declare val as static and yes I do know what a static variable is but do you mean threads of a same class only share global variables???
And why do you say my threads are doing nothing? I see each thread displaying a message with its own id so I suspect that 2 different threads runs at the same time, wrong?

Hi,

Quote:

---

do you mean threads of a same class only share global variables???

---

All objects of a class share their static variables but not the other ones. Every object of a class have a copy of the variable if it is not defined as static.

Quote:

---

And why do you say my threads are doing nothing?

---

You have to redefine the "run()" method and do whaterver you want to do there.
If you want to each thread have it's event loop to make that a slot is executed from the thread you have to call "exec()" into the "run()" method.
Another way to use a thread is to use a loop into the "run()" method to do a heavy process, ...

I recommend you to take a look at Qt thread examples.

Quote:

---

Originally Posted by Caius Aérobus

What you did is declare val as static and yes I do know what a static variable is but do you mean threads of a same class only share global variables???

---

There is no such concept as "threads of the same class".

Quote:

---

I see each thread displaying a message with its own id so I suspect that 2 different threads runs at the same time, wrong?

---

What happens if you do not call start() on the threads at all? If it were the threads displaying those messages, you should not be getting them at all, right?

Quote:

---

Originally Posted by ^NyAw^

Hi,


All objects of a class share their static variables but not the other ones. Every object of a class have a copy of the variable if it is not defined as static.


You have to redefine the "run()" method and do whaterver you want to do there.
If you want to each thread have it's event loop to make that a slot is executed from the thread you have to call "exec()" into the "run()" method.
Another way to use a thread is to use a loop into the "run()" method to do a heavy process, ...

I recommend you to take a look at Qt thread examples.

---

a QThread will call exec() in run() by default unless it is overridden.

Ok I think I start to understand how things go. I have modified the code (see below), all threads are running (see prints) but only the main thread executes the slot so what is wrong now?

Code:

---

class buttonThread : public QThread
{
  Q_OBJECT
 
public:
  buttonThread(QWidget *parent = 0, int number = 0);
 
public:
  void run();
 
public slots:
  void threadClicked();
 
private:
  QMessageBox *msg;
  QVector<long> thread_ids;
  static int val;
  int GetId();
};
 
class buttons : public QWidget
{
  Q_OBJECT
 
public:
  buttons(QWidget *parent = 0);
 
private:
  QPushButton *pushb[2];
  buttonThread *threads[2];
};
 
int buttonThread::val = 0;
 
buttons::buttons(QWidget *parent)
{
  printf("buttons(): QThread::currentThread() = %p\n", QThread::currentThread());
  QHBoxLayout *layout = new QHBoxLayout(this);
  this->pushb[0] = new QPushButton("Thread 1", this);
  layout->addWidget(this->pushb[0]);
  threads[0] = new buttonThread(this, 1);
  QObject::connect(this->pushb[0], SIGNAL(clicked()), threads[0], SLOT(threadClicked()));
  this->pushb[1] = new QPushButton("Thread 2", this);
  layout->addWidget(this->pushb[1]);
  threads[1] = new buttonThread(this, 2);
  QObject::connect(this->pushb[1], SIGNAL(clicked()), threads[1], SLOT(threadClicked()));
  threads[0]->start();
  threads[1]->start();
}
 
buttonThread::buttonThread(QWidget *parent, int number)
{
  this->thread_ids.resize(2);
  this->thread_ids[number-1] = (long)QThread::currentThread();
  this->val = 0;
}
 
int
buttonThread::GetId()
{
  printf("GetId(): QThread::currentThread() = %p\n", QThread::currentThread());
  return this->thread_ids.indexOf((long)QThread::currentThread()) + 1;
}
 
void
buttonThread::threadClicked()
{
  QMessageBox *msg = new QMessageBox(QMessageBox::NoIcon, QString(), QString("Thread %1 : val = %2").arg(this->GetId()).arg(this->val), QMessageBox::Ok);
  this->val++;
  msg->exec();
}
 
void
buttonThread::run()
{
  printf("run(): QThread::currentThread() = %p\n", QThread::currentThread());
  this->exec();
}

---

Code:

---

buttons(): QThread::currentThread() = 0x101300e90
run(): QThread::currentThread() = 0x1011070b0
run(): QThread::currentThread() = 0x10114fc70
GetId(): QThread::currentThread() = 0x101300e90
GetId(): QThread::currentThread() = 0x101300e90
GetId(): QThread::currentThread() = 0x101300e90

---

The OP

Quote:

---

The expected behaviour of my code is both threads increasing a shared variable named val.

---

So your problem is solved.

Quote:

---

I have modified the code (see below), all threads are running (see prints) but only the main thread executes the slot so what is wrong now?

---

The word "wrong" is very subjective. IMO as this exercise is to understand QThread, I will say if the code does what you wanted it do then it not "worng".

Hi,

Don't create GUI elements on another thread that is not the main thread, so forget to create a QMessageBox on the thread slot. This will get you app crash.
Use "Qt::QueuedConnection" as the last parameter of the connection between the signal and the slot.

Quote:

---

Originally Posted by Caius Aérobus

Ok I think I start to understand how things go. I have modified the code (see below), all threads are running (see prints) but only the main thread executes the slot so what is wrong now?

---

Your QThread object still lives in the main thread. You have to treat an instance of QThread not as a thread itself but rather as an object controlling a thread. This object has no real relation to the thread it controls apart the fact that when the thread is started, it executes the run() method of the QThread instance.

Read this article for more details: http://blog.qt.digia.com/blog/2010/0...oing-it-wrong/

Quote:

---

Originally Posted by wysota

Read this article for more details: http://blog.qt.digia.com/blog/2010/0...oing-it-wrong/

---

Great article! Thanks a lot for this hint, it has helped me to understand how things have been made for.
Below my final code, probably not as you would have written it but quite closer to what it should.

Code:

---

class buttonThread : public QThread
{
  Q_OBJECT
 
public:
  buttonThread(QWidget *parent = 0) {};
  void run();
};
 
class message : public QObject
{
  Q_OBJECT
 
public:
  message(QWidget *parent = 0) {};
 
signals:
  void buttonHasBeenClicked(buttonThread *, int);
 
public slots:
  void threadClicked();
};
 
class buttons : public QWidget
{
  Q_OBJECT
 
public:
  buttons(QWidget *parent = 0);
 
public slots:
  void buttonClicked(buttonThread *current, int);
 
private:
  QVector<QPushButton *> pushb;
  QVector<buttonThread *> threads;
  QVector<message *> msg;
};
 
#define NBTHREADS 2
 
int val = 0;
 
buttons::buttons(QWidget *parent)
{
  printf("buttons(): QThread::currentThread() = %p\n", QThread::currentThread());
  QHBoxLayout *layout = new QHBoxLayout(this);
  for (int i=0 ; i<NBTHREADS ; i++) {
    // Button
    this->pushb << new QPushButton(QString("Thread %1").arg(i), this);
    layout->addWidget(this->pushb[i]);
    this->msg << new message(this);
    QObject::connect(this->pushb[i], SIGNAL(clicked()), this->msg[i], SLOT(threadClicked()));
    QObject::connect(this->msg[i], SIGNAL(buttonHasBeenClicked(buttonThread *, int)), this, SLOT(buttonClicked(buttonThread *, int)));
    // Thread
    this->threads << new buttonThread(this);
    this->msg[i]->moveToThread(this->threads[i]);
    // Start thread
    this->threads[i]->start();
  }
}
 
void
buttons::buttonClicked(buttonThread *current, int value)
{
  int id = this->threads.indexOf(current);
  if (id == -1) {
    QMessageBox *msg = new QMessageBox(QMessageBox::NoIcon, QString("Error"), QString("buttons::buttonClicked: current=%1 not found in thread list").arg((long)current, 0, 16), QMessageBox::Ok);
    msg->exec();
    return;
  }
  QMessageBox *msg = new QMessageBox(QMessageBox::NoIcon, QString("Button clicked"), QString("button %1 clicked - val = %2").arg(id).arg(value), QMessageBox::Ok);
  msg->exec();
}
 
void
message::threadClicked()
{
  printf("threadClicked(): QThread::currentThread() = %p\n", QThread::currentThread());
  emit buttonHasBeenClicked((buttonThread *)QThread::currentThread(), val++);
}
 
void
buttonThread::run()
{
  printf("run(): QThread::currentThread() = %p\n", QThread::currentThread());
  this->exec();
}

---