From 1f5e0f649484a033386e7607c3b0f8732e6ccfd4 Mon Sep 17 00:00:00 2001 From: Joao Matos Date: Sun, 18 May 2025 13:23:08 +0100 Subject: [PATCH] App: Invoke `signalBeforeRecompute()` on the GUI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Historically, `App::Document::recompute()` ran entirely on the **main** (GUI) thread and directly emitted `signalBeforeRecompute()`. * Add-ons like **Assembly3** and others depend on that signal for setup/teardown hooks before any recompute work begins. * After offloading `recompute()` into a background worker thread to keep the UI responsive, calling `signalBeforeRecompute()` directly from the worker would break thread-affinity rules and silently break compatibility with those add-ons. **Solution** 1. **Introduce a generic hook** (`PreRecomputeHook`) in **App::Document**: * A `std::function` that, if set, is invoked at the very start of `recompute()`. * Core code stays Qt-free—only knows to call a callback if one exists. 2. **Wire up the hook in `Gui::Document`**: * In the GUI wrapper’s constructor, install a hook that calls `callSignalBeforeRecompute()`. * `callSignalBeforeRecompute()` uses `QMetaObject::invokeMethod(..., Qt::BlockingQueuedConnection)` to enqueue `signalBeforeRecompute()` on the GUI thread and **block** the worker until it completes. * If already on the GUI thread, it simply calls the signal directly. 3. **Maintain add-on compatibility**: * From the add-on’s perspective nothing changes—they still receive `signalBeforeRecompute()` on the main thread before any recompute work. * Internally, the recompute body now runs on a worker thread, improving UI responsiveness without breaking existing hooks. **Result** * **Recompute** remains fully backward-compatible for add-ons like Assembly3. * **UI thread** still handles all GUI-related signaling. * **Worker thread** performs the actual heavy lifting, unblocked only once the GUI is primed and all pre-recompute signals have been delivered. --- src/App/Document.cpp | 13 ++++++++++++- src/App/Document.h | 2 ++ src/App/private/DocumentP.h | 2 ++ src/Gui/Document.cpp | 22 ++++++++++++++++++++++ src/Gui/Document.h | 1 + 5 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/App/Document.cpp b/src/App/Document.cpp index dc821048b2..cbdf1b23ab 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -2878,6 +2878,11 @@ void Document::renameObjectIdentifiers( } } +void Document::setPreRecomputeHook(const PreRecomputeHook& hook) +{ + d->_preRecomputeHook = hook; +} + int Document::recompute(const std::vector& objs, bool force, bool* hasError, @@ -2918,7 +2923,13 @@ int Document::recompute(const std::vector& objs, FC_TIME_INIT(t); Base::ObjectStatusLocker exe(Document::Recomputing, this); - signalBeforeRecompute(*this); + + // This will hop into the main thread, fire signalBeforeRecompute(), + // and *block* the worker until the main thread is done, avoiding races + // between any running Python code and the rest of the recompute call. + if (d->_preRecomputeHook) { + d->_preRecomputeHook(); + } ////////////////////////////////////////////////////////////////////////// // FIXME Comment by Realthunder: diff --git a/src/App/Document.h b/src/App/Document.h index b78baa23f0..0c63005b58 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -193,6 +193,8 @@ public: //@} // NOLINTEND + using PreRecomputeHook = std::function; + void setPreRecomputeHook(const PreRecomputeHook& hook); void clearDocument(); diff --git a/src/App/private/DocumentP.h b/src/App/private/DocumentP.h index eac25f6633..924a1c5430 100644 --- a/src/App/private/DocumentP.h +++ b/src/App/private/DocumentP.h @@ -98,6 +98,8 @@ struct DocumentP StringHasherRef Hasher {new StringHasher}; + Document::PreRecomputeHook _preRecomputeHook; + DocumentP(); void addRecomputeLog(const char* why, App::DocumentObject* obj) diff --git a/src/Gui/Document.cpp b/src/Gui/Document.cpp index 2b0f171bf7..7182eaae31 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -39,6 +39,7 @@ # include # include # include +# include # include # include # include @@ -505,6 +506,8 @@ Document::Document(App::Document* pcDocument,Application * app) (std::bind(&Gui::Document::slotTransactionRemove, this, sp::_1, sp::_2)); //NOLINTEND + pcDocument->setPreRecomputeHook([this] { callSignalBeforeRecompute(); }); + // pointer to the python class // NOTE: As this Python object doesn't get returned to the interpreter we // mustn't increment it (Werner Jan-12-2006) @@ -1193,6 +1196,25 @@ void Document::slotTouchedObject(const App::DocumentObject &Obj) } } +// helper that guarantees signalBeforeRecompute call is executed in the GUI thread and +// that the worker waits until it finishes +void Document::callSignalBeforeRecompute() +{ + auto invokeSignalBeforeRecompute = [this]{ + // this runs in the GUI thread + this->getDocument()->signalBeforeRecompute(*this->getDocument()); + }; + + if (QThread::currentThread() == qApp->thread()) { + // already on GUI thread – no hop, just call it + invokeSignalBeforeRecompute(); + } else { + // hop to GUI and *block* until it returns + QMetaObject::invokeMethod(qApp, std::move(invokeSignalBeforeRecompute), + Qt::BlockingQueuedConnection); + } +} + void Document::addViewProvider(Gui::ViewProviderDocumentObject* vp) { // Hint: The undo/redo first adds the view provider to the Gui diff --git a/src/Gui/Document.h b/src/Gui/Document.h index 4852f6bcd6..ae27120a6b 100644 --- a/src/Gui/Document.h +++ b/src/Gui/Document.h @@ -94,6 +94,7 @@ protected: void slotSkipRecompute(const App::Document &doc, const std::vector &objs); void slotTouchedObject(const App::DocumentObject &); void slotChangePropertyEditor(const App::Document&, const App::Property &); + void callSignalBeforeRecompute(); //@} public: