From 79e87cb3b844fe0fdbef280654cde7d04a00c497 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Wed, 30 Sep 2026 10:35:19 +1300 Subject: [PATCH] Set up the elements collection's items on the GUI thread ElementsCollectionModel::loadCollections() runs setUpData() for every item on worker threads (QtConcurrent::map), and setUpData() calls setText(), setFlags(), setData() and setToolTip() on items that are already in the model. Each of these changes the model and emits its dataChanged() signal from a worker thread, which QAbstractItemModel does not allow. Loading the shipped collection (8838 elements) emitted dataChanged() 38958 times, all from worker threads. The expensive part (reading every element file) stays on the worker threads. Only the result is moved: ElementCollectionItem::setData() keeps a value set from a worker thread on the item, data() returns it to the same worker so setUpData() still reads back what it has set, and the model applies the kept values on the GUI thread when the map is finished, before emitting loadingFinished(). setUpData() called on the GUI thread (macros collection, a single added or changed element) is unchanged. With this change the same load emits dataChanged() 38958 times, all on the GUI thread. Revives the still-needed part of #516, closed only to clear a review backlog. Its other two changes are left out: the wait in loadMacrosCollection() guarded a model shared with a running map, which no longer happens (the macros always get a model of their own), and qetinformation.h's static QString constants are a size clean-up, not a bug. Co-Authored-By: Claude Opus 5.5 --- .../elementcollectionitem.cpp | 70 +++++++++++++++++++ .../elementcollectionitem.h | 11 +++ .../elementscollectionmodel.cpp | 11 ++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/sources/ElementsCollection/elementcollectionitem.cpp b/sources/ElementsCollection/elementcollectionitem.cpp index 3e63c8c8c..ac63291b6 100644 --- a/sources/ElementsCollection/elementcollectionitem.cpp +++ b/sources/ElementsCollection/elementcollectionitem.cpp @@ -18,6 +18,9 @@ #include "elementcollectionitem.h" +#include +#include + /** @brief ElementCollectionItem::ElementCollectionItem Constructor @@ -247,6 +250,73 @@ QList ElementCollectionItem::items() const return list; } +/** + @brief onGuiThread + @return true when called from the thread the application lives in +*/ +static bool onGuiThread() +{ + return QThread::currentThread() == QCoreApplication::instance()->thread(); +} + +/** + @brief ElementCollectionItem::setData + ElementsCollectionModel::loadCollections() runs setUpData() for every + item on worker threads (QtConcurrent::map), while the items are + already in the model. QStandardItem::setData() updates the model and + emits its dataChanged() signal, which is not safe from a worker + thread. So a value set from a worker thread is kept on the item, and + the model applies it on the GUI thread with applyDeferredData() once + every item is set up. setText(), setFlags(), setToolTip() and setIcon() + all end up here. + @param value + @param role +*/ +void ElementCollectionItem::setData(const QVariant &value, int role) +{ + if (role == Qt::EditRole) + role = Qt::DisplayRole; + + if (onGuiThread()) + QStandardItem::setData(value, role); + else + m_deferred_data.insert(role, value); +} + +/** + @brief ElementCollectionItem::data + On a worker thread, a value set by setData() but not applied yet is + returned, so setUpData() reads back what it has just set + (localName() tests text() for example). + @param role + @return +*/ +QVariant ElementCollectionItem::data(int role) const +{ + if (role == Qt::EditRole) + role = Qt::DisplayRole; + + if (!onGuiThread()) + { + const auto it = m_deferred_data.constFind(role); + if (it != m_deferred_data.constEnd()) + return it.value(); + } + return QStandardItem::data(role); +} + +/** + @brief ElementCollectionItem::applyDeferredData + Apply the values set from a worker thread, see setData(). + Must be called on the GUI thread, after the worker is done. +*/ +void ElementCollectionItem::applyDeferredData() +{ + for (auto it = m_deferred_data.constBegin() ; it != m_deferred_data.constEnd() ; ++it) + QStandardItem::setData(it.value(), it.key()); + m_deferred_data.clear(); +} + void setUpData(ElementCollectionItem *eci) { eci->setUpData(); } diff --git a/sources/ElementsCollection/elementcollectionitem.h b/sources/ElementsCollection/elementcollectionitem.h index ad12477c1..057215662 100644 --- a/sources/ElementsCollection/elementcollectionitem.h +++ b/sources/ElementsCollection/elementcollectionitem.h @@ -18,6 +18,7 @@ #ifndef ELEMENTCOLLECTIONITEM2_H #define ELEMENTCOLLECTIONITEM2_H +#include #include /** @@ -56,6 +57,16 @@ class ElementCollectionItem : public QStandardItem QList elementsChild() const; QList directoriesChild() const; QList items() const; + + QVariant data(int role = Qt::UserRole + 1) const override; + void setData(const QVariant &value, int role = Qt::UserRole + 1) override; + void applyDeferredData(); + + private: + /// Values set by setUpData() while it runs on a worker thread, + /// see setData(). Only ever touched by that one worker thread, + /// until applyDeferredData() empties it on the GUI thread. + QHash m_deferred_data; }; void setUpData(ElementCollectionItem *eci); diff --git a/sources/ElementsCollection/elementscollectionmodel.cpp b/sources/ElementsCollection/elementscollectionmodel.cpp index 00ef0d2eb..440f10417 100644 --- a/sources/ElementsCollection/elementscollectionmodel.cpp +++ b/sources/ElementsCollection/elementscollectionmodel.cpp @@ -312,8 +312,15 @@ void ElementsCollectionModel::loadCollections(bool common_collection, this, &ElementsCollectionModel::loadingProgressValueChanged); connect(watcher, &QFutureWatcher::progressRangeChanged, this, &ElementsCollectionModel::loadingProgressRangeChanged); - connect(watcher, &QFutureWatcher::finished, - this, &ElementsCollectionModel::loadingFinished); + //setUpData() ran on worker threads, which only kept the values on + //the items (ElementCollectionItem::setData()): apply them here, on + //the GUI thread, before anyone is told the loading is finished. + connect(watcher, &QFutureWatcher::finished, this, [this]() + { + for (ElementCollectionItem *eci : std::as_const(m_items_list_to_setUp)) + eci->applyDeferredData(); + emit loadingFinished(); + }); connect(watcher, &QFutureWatcher::finished, watcher, &QFutureWatcher::deleteLater);