Is this an acceptable/safe way to update GUI from another thread?

168gr

I am new to Qt and am writing a program on Windows, using Visual Studio Community 2015 and the Qt Designer plugin.

One of the things this program does is create a background thread to monitor some external hardware. I used the WINAPI CreateThread for this, not a Qthread, because a prior version of the program was not GUI based, and I'd rather avoid rewriting anything I don't have to.

I want the main GUI window to update when the external hardware state changes, which may be rarely, or several times per second.

I understand that it is unsafe to update the Qt GUI from worker threads, and that the main GUI thread should be signaled to do this work itself. This is the method that I am using. It works, but I want to

confirm that it is indeed safe
ask if there is a better/cleaner way to do it

Stripped down to its bones, this is my method:

In my main QMainWindow class, I've added a function to do the actual GUI update

class MyProg : public QMainWindow
  [...]
private:
  Ui::MyProgClass ui;

  [...]
public slots:
  void gui_update();

MyProg::MyProg(QWidget *parent)
	: QMainWindow(parent)
{
  MyProg *my_prog_pointer;
  QGraphicsScene * scene = new QGraphicsScene();

  ui.setupUi(this);

  my_prog_pointer = this;

my_prog_pointer is ultimately passed as a parameter to the worker thread, so that thread will know who to notify when GUI work has to be done.

The GUI update function is something like:

void MyProg::gui_update() {
  ui.msg->setText("blah");
}

And from the other thread, when I want to update the GUI, I call

QMetaObject::invokeMethod(my_prog_pointer, "gui_update", Qt::QueuedConnection);

And it works.

Is this method safe? If so, is there a reason (performance, style, whatever) to do it differently?

Thank you.

SGaist

Hi and welcome to devnet,

You're complicating things a bit.

Your worker object should have a signal that you emit at some point and you should connect that signal to the qui_update slot.

ambershark

@168gr It is not safe. You can't really pass the pointer to a new thread without protecting it with thread synchronization objects of some sort. Like a mutex.

Even then it is still not the way you want to do it. You want to have a signal from your thread that is registered to a slot on your gui thread. Exactly like @SGaist said.

168gr

@ambershark @ambershark

Thank you - couple followup questions

One, I'm not exactly clear why there'd be a need for mutex or other synchronization methods here. Doesn't the use of the QueuedConnection parameter in the invokeMethod function direct the GUI thread to simply add the update request to its queue of tasks? The pointer to the GUI object doesn't ever change.

QMetaObject::invokeMethod(my_prog_pointer, "gui_update", Qt::QueuedConnection);

Two, 98% of my Qt code was generated by the Qt Designer plugin. It has a signal/slot editor but it appears to only have the capability of selecting a GUI feature as a "sender" ... and my worker thread needs to be able to send it.

Your worker object should have a signal that you emit at some point and you should connect that signal to the qui_update slot.

This may be the most trivial and basic of Qt questions, but is it possible to send a signal from a function that isn't part of a QObject? The documentation gives a brief example

  class Counter : public QObject

  [...]

  Counter a, b;
  QObject::connect(
    &a, SIGNAL(valueChanged(int)),
    &b, SLOT(setValue(int)));

Is the correct answer to reimplement my WINAPI thread as a Qthread and use connect() to send signals from it to the GUI thread?

I am reluctant to go messing around with worker threads that have been functional, debugged, and essentially unchanged through a couple of pre-GUI versions of the program. :( But if that's the right answer ...

Thank you.

SGaist

What does that thread do ?

168gr

@SGaist said in Is this an acceptable/safe way to update GUI from another thread?:

What does that thread do ?

Continuously monitors some external hardware over an RS232 port. GUI should be updated with its state whenever it changes.

ambershark

@168gr said in Is this an acceptable/safe way to update GUI from another thread?:

@ambershark @ambershark

Thank you - couple followup questions

One, I'm not exactly clear why there'd be a need for mutex or other synchronization methods here. Doesn't the use of the QueuedConnection parameter in the invokeMethod function direct the GUI thread to simply add the update request to its queue of tasks? The pointer to the GUI object doesn't ever change.

So it's not really whether or not the pointer changes but whether or not the data it is pointing to changes. If you are 100% sure that the class is never modified by both the worker and gui thread then you will be ok without the mutex.

In my experience this is rarely the case and inevitably (even if it's down the line), someone uses that pointer in a way that changes the data and you end up with a threading bug which is absolutely miserable to track down.

I did make the assumption that since you passed a non-const pointer that it was being used to modify something. Although you can still run into sync issues even if you are just reading in the thread if you happen to be mid read when the other thread writes to it.

If you are just using it to place the function call on the event stack you should be ok. I wouldn't necessarily trust it, but I'm pretty paranoid when it comes to threading. Once you've spent a month on a threading bug you'll never ever want to go without proper synchronization again. ;)

QMetaObject::invokeMethod(my_prog_pointer, "gui_update", Qt::QueuedConnection);
Two, 98% of my Qt code was generated by the Qt Designer plugin. It has a signal/slot editor but it appears to only have the capability of selecting a GUI feature as a "sender" ... and my worker thread needs to be able to send it.

Your worker object should have a signal that you emit at some point and you should connect that signal to the qui_update slot.

This may be the most trivial and basic of Qt questions, but is it possible to send a signal from a function that isn't part of a QObject? The documentation gives a brief example
  class Counter : public QObject

  [...]

  Counter a, b;
  QObject::connect(
    &a, SIGNAL(valueChanged(int)),
    &b, SLOT(setValue(int)));
Is the correct answer to reimplement my WINAPI thread as a Qthread and use connect() to send signals from it to the GUI thread?

AFAIK you can not use signals/slots without a QObject. In order to use signal or any of the slots keywords in the class and have them moc properly you need the Q_OBJECT macro. In order to use that macro you must be derived from a QObject.

There may be some edge case here I don't know about though. But that has always been my experience with Qt.

I am reluctant to go messing around with worker threads that have been functional, debugged, and essentially unchanged through a couple of pre-GUI versions of the program. :( But if that's the right answer ...

Totally understandable. If what you have is working for you and you've tested it thoroughly, on different cpus especially, then you should be ok. Even if it isn't the cleanest way to do it, if it's not broken you don't necessarily need to fix it. ;)

Multiple CPU testing is important though as most threading issues don't show up until you change the timing.

Thank you.

No problem. :)