Fix of the fix: avoid direct manipulation of GUI elements by non-GUI threads

Author: joern274

plugins/dataflow_analysis/include/dataflow_analysis/utils/progress_printer.h

@@ -38,7 +38,7 @@ namespace hal
 
             void print_progress_to_stderr(float progress, const std::string& message = "");
 
-            // GUI calls must not be called from semaphore/mutex protected scope, otherwise threads might block each other
+            // Safe to call from any thread, any lock state: the registered indicator posts a queued event to the GUI.
             void print_progress_to_gui(int percent = -1); // use m_last_percentage if negative
 
             void print_message_to_gui(const std::string& message);

plugins/dataflow_analysis/src/utils/progress_printer.cpp

@@ -41,10 +41,7 @@ namespace hal
             if (!GuiExtensionDataflow::s_progress_indicator_function) return;
             if (percent < 0) percent = m_last_percentage;
             if (percent > 99) percent = 99;
-            {
-                GuiExtensionDataflow::s_progress_indicator_function(percent, m_gui_message);
-            }
-
+            GuiExtensionDataflow::s_progress_indicator_function(percent, m_gui_message);
         }
 
         void ProgressPrinter::print_progress_to_stderr(float progress, const std::string& message)

plugins/gui/include/gui/graph_widget/progress_bar.h

@@ -31,7 +31,6 @@
 #include <QPushButton>
 #include <QImage>
 #include <QTimer>
-#include <QMutex>
 
 namespace hal {
 
@@ -65,14 +64,12 @@ namespace hal {
         Q_OBJECT
         QImage mImage;
         double mAngle;
-        QMutex* mMutex;
     public Q_SLOTS:
         void handleTimeout();
     protected:
         void paintEvent(QPaintEvent* event) override;
     public:
         BusyAnimation(QWidget* parent = nullptr);
-        ~BusyAnimation();
     };
 
     class BusyIndicator : public AbstractBusyIndicator
@@ -81,7 +78,6 @@ namespace hal {
         QTimer* mTimer;
         QLabel* mLabel;
         QProgressBar* mProgressBar;
-        QMutex* mMutex;
     public:
         BusyIndicator(QWidget* parent = nullptr);
         ~BusyIndicator();

plugins/gui/src/graph_widget/graph_widget.cpp

@@ -39,6 +39,7 @@
 #include <QInputDialog>
 #include <QKeyEvent>
 #include <QMessageBox>
+#include <QThread>
 #include <QTimer>
 #include <QToolButton>
 #include <QVBoxLayout>
@@ -148,7 +149,10 @@ namespace hal
         if (!text.isEmpty())
             mProgressBar->setText(text);
         mProgressBar->setValue(percent);
-        qApp->processEvents();
+		if (QThread::currentThread() == qApp->thread())
+        {
+            qApp->processEvents();
+        }
     }
 
     void GraphWidget::showComments(const Node &nd)
@@ -1066,12 +1070,25 @@ namespace hal
 
     void GraphWidget::pluginProgressIndicator(int percent, const std::string& msg)
     {
-        // qDebug() << "progress" << hex << (quintptr) QThread::currentThread() << (quintptr) gPythonContext->currentThread() << (quintptr) dynamic_cast<PythonThread*>(QThread::currentThread());
-        if (!sInstance || gPythonContext->pythonThread()) return;
-        if (percent==100)
-            sInstance->handleSceneAvailable();
+        // Qt widgets must only be touched on the GUI thread. If we're already on it,
+        // call showBusy / handleSceneAvailable directly; otherwise post the update to
+        // the main event queue.
+        QString qmsg = QString::fromStdString(msg);
+        auto apply = [percent, qmsg]() {
+            if (!sInstance) return;
+            if (percent == 100)
+                sInstance->handleSceneAvailable();
+            else
+                sInstance->showBusy(percent, qmsg);
+        };
+        if (QThread::currentThread() == qApp->thread())
+        {
+            apply();
+            qApp->processEvents();
+        }
+
         else
-            sInstance->showBusy(percent, QString::fromStdString(msg));
+            QMetaObject::invokeMethod(qApp, apply, Qt::QueuedConnection);
     }
 
 }    // namespace hal

plugins/gui/src/graph_widget/progress_bar.cpp

@@ -46,7 +46,6 @@ namespace hal {
     BusyIndicator::BusyIndicator(QWidget* parent)
         : AbstractBusyIndicator(parent)
     {
-        mMutex = new QMutex;
         QVBoxLayout* layout = new QVBoxLayout(this);
         layout->setAlignment(Qt::AlignHCenter);
         layout->addWidget(mLabel = new QLabel(this));
@@ -61,32 +60,23 @@ namespace hal {
     BusyIndicator::~BusyIndicator()
     {
         mTimer->stop();
-        delete mMutex;
     }
 
     BusyAnimation::BusyAnimation(QWidget* parent)
-        : QWidget(parent), mImage(QImage(":/icons/hal9000","PNG")), mAngle(0), mMutex(new QMutex)
+        : QWidget(parent), mImage(QImage(":/icons/hal9000","PNG")), mAngle(0)
     {;}
 
-    BusyAnimation::~BusyAnimation()
-    {
-        delete mMutex;
-    }
-
     void BusyAnimation::handleTimeout()
     {
         mAngle += 5;
         update();
-        qApp->processEvents();
     }
 
     void BusyAnimation::paintEvent(QPaintEvent* event)
     {
         Q_UNUSED(event);
-        if (mMutex->try_lock())
-        {
-            QPainter p(this);
-            p.setRenderHint(QPainter::Antialiasing);
+        QPainter p(this);
+        p.setRenderHint(QPainter::Antialiasing);
 
             int rw = rect().width();
             int rh = rect().height();
@@ -116,25 +106,20 @@ namespace hal {
                     }
                 }
 
-            p.drawImage(rimg,img);
-            mMutex->unlock();
-        }
+        p.drawImage(rimg,img);
     }
 
     void BusyIndicator::setValue(int percent)
     {
-        if (mProgressBar->value() == percent) return; // nothing to do
-        QMutexLocker lock(mMutex);
+        if (mProgressBar->value() == percent) return;
         mProgressBar->setValue(percent);
         update();
-        qApp->processEvents();
     }
 
     void BusyIndicator::setText(const QString &txt)
     {
-        if (mLabel->text() == txt) return; // nothing to do
-        QMutexLocker lock(mMutex);
+        if (mLabel->text() == txt) return;
         mLabel->setText(txt);
-        qApp->processEvents();
+        update();
     }
 }