fix: Refactor DConfig wrapper class generation for thread safety and … #531
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lifecycle management Problem: - Single-threaded design with weak state machine (Invalid -> Succeed/Failed) - No proper handling of object destruction during initialization - Signal emissions in worker thread context (incorrect thread context) - Fragile destructor unable to handle all cleanup scenarios Solution: 1. Introduce Data layer separation (TreelandUserConfigData + TreelandUserConfig) - Clear separation between internal data management and public API - Enables safer object lifecycle management 2. Enhance state machine (3-state -> 5-state model) - Add Initializing and Destroyed states - Use atomic CAS operations for thread-safe state transitions - States: Invalid -> Initializing -> (Succeed | Failed | Destroyed) 3. Improve async initialization and cleanup - Use QPointer for safe backref checks (prevent use-after-free) - Support 4 destruction paths: normal/failed/quick/mid-initialization - Atomic state transitions with proper signal emission guards 4. Separate thread responsibilities - updateValue(): Worker thread reads config values - updateProperty(): Main thread updates properties and emits signals - Use QMetaObject::invokeMethod for correct thread context Improvements: - Thread safety: Complete atomic operations coverage - Memory safety: QPointer guards prevent dangling pointers - Code clarity: Layered architecture with clear responsibilities - Backward compatibility: API unchanged
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
deepin-ci-robot
added a commit
to linuxdeepin/dtk6core
that referenced
this pull request
Jan 7, 2026
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Author
--- treelandconfig-old.hpp 2026-01-07 15:43:28.000000000 +0800
+++ treelandconfig-new.hpp 2026-01-07 14:35:13.000000000 +0800
@@ -1,7 +1,7 @@
/**
* This file is generated by dconfig2cpp.
* Command line arguments: /usr/libexec/dtk6/DCore/bin/dconfig2cpp -o /home/uos/treeland/treeland/build/src/treelandconfig.hpp -c TreelandConfig /home/uos/treeland/treeland/misc/dconfig/org.deepin.dde.treeland.json
- * Generation time: 2026-01-07T15:43:28
+ * Generation time: 2026-01-07T14:35:13
* JSON file version: 1.0
*
* WARNING: DO NOT MODIFY THIS FILE MANUALLY.
@@ -23,8 +23,58 @@
#include <DSGApplication>
#include <DConfig>
+class TreelandConfig;
+
+class TreelandConfigData : public QObject {
+public:
+ enum class Status {
+ Invalid = 0,
+ Initializing = 1,
+ Succeed = 2,
+ Failed = 3,
+ Destroyed = 4
+ };
+
+ explicit TreelandConfigData(QObject *parent = nullptr)
+ : QObject(parent) {}
+
+ void initializeInConfigThread(DTK_CORE_NAMESPACE::DConfig *config);
+ void updateValue(const QString &key, const QVariant &fallback = QVariant());
+ void updateProperty(const QString &key, const QVariant &value);
+
+ inline void markPropertySet(const int index, bool on = true) {
+ if (index < 32) {
+ if (on)
+ m_propertySetStatus0.fetchAndOrOrdered(1 << (index - 0));
+ else
+ m_propertySetStatus0.fetchAndAndOrdered(~(1 << (index - 0)));
+ return;
+ }
+ Q_UNREACHABLE();
+ }
+
+ inline bool testPropertySet(const int index) const {
+ if (index < 32) {
+ return (m_propertySetStatus0.loadRelaxed() & (1 << (index - 0)));
+ }
+ Q_UNREACHABLE();
+ }
+
+ QAtomicPointer<DTK_CORE_NAMESPACE::DConfig> m_config = nullptr;
+ QAtomicInteger<int> m_status = static_cast<int>(Status::Invalid);
+ QPointer<TreelandConfig> m_userConfig = nullptr;
+ QAtomicInteger<quint32> m_propertySetStatus0 = 0;
+
+ // Property storage
+ bool p_enablePrelaunchSplash { false };
+ bool p_forceSoftwareCursor { false };
+ qlonglong p_prelaunchSplashTimeoutMs { 5000 };
+};
+
class TreelandConfig : public QObject {
Q_OBJECT
+public:
+ using Data = TreelandConfigData;
Q_PROPERTY(bool enablePrelaunchSplash READ enablePrelaunchSplash WRITE setEnablePrelaunchSplash NOTIFY enablePrelaunchSplashChanged RESET resetEnablePrelaunchSplash)
Q_PROPERTY(bool forceSoftwareCursor READ forceSoftwareCursor WRITE setForceSoftwareCursor NOTIFY forceSoftwareCursorChanged RESET resetForceSoftwareCursor)
@@ -37,15 +87,25 @@
explicit TreelandConfig(QThread *thread, DTK_CORE_NAMESPACE::DConfigBackend *backend,
const QString &name, const QString &appId, const QString &subpath,
bool isGeneric, QObject *parent)
- : QObject(nullptr) {
+ : QObject(parent), m_data(new Data) {
+ m_data->m_userConfig = this;
+
if (!thread->isRunning()) {
qWarning() << QLatin1String("Warning: The provided thread is not running.");
}
Q_ASSERT(QThread::currentThread() != thread);
auto worker = new QObject();
worker->moveToThread(thread);
- QPointer<QObject> watcher(parent);
- QMetaObject::invokeMethod(worker, [=, this]() {
+ auto data = m_data;
+
+ QMetaObject::invokeMethod(worker, [=]() {
+ // Atomically transition from Invalid to Initializing
+ if (!data->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid),
+ static_cast<int>(Data::Status::Initializing))) {
+ worker->deleteLater();
+ return;
+ }
+
DTK_CORE_NAMESPACE::DConfig *config = nullptr;
if (isGeneric) {
if (backend) {
@@ -70,20 +130,37 @@
}
}
}
- if (!config) {
+
+ if (!config || !config->isValid()) {
qWarning() << QLatin1String("Failed to create DConfig instance.");
+
+ if (data->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Initializing),
+ static_cast<int>(Data::Status::Failed))) {
+ // Successfully transitioned to Failed - notify main thread
+ QMetaObject::invokeMethod(data, [data]() {
+ if (data->m_userConfig)
+ Q_EMIT data->m_userConfig->configInitializeFailed();
+ });
+ }
+
worker->deleteLater();
+ if (config)
+ delete config;
+
return;
}
+
config->moveToThread(QThread::currentThread());
- initializeInConfigThread(config);
- if (watcher != parent) {
- // delete this if watcher is changed to nullptr.
- deleteLater();
- } else if (!this->parent() && parent) {
- // !parent() means that parent is not changed.
- this->setParent(watcher);
- }
+
+ // Initialize through Data class
+ data->initializeInConfigThread(config);
+
+ QObject::connect(config, &DTK_CORE_NAMESPACE::DConfig::valueChanged, data, [data](const QString &key) { data->updateValue(key); }, Qt::DirectConnection);
+ QObject::connect(config, &QObject::destroyed, data, &QObject::deleteLater);
+ QMetaObject::invokeMethod(data, [data, config]() {
+ if (data->m_userConfig)
+ Q_EMIT data->m_userConfig->configInitializeSucceed(config);
+ });
worker->deleteLater();
});
}
@@ -103,261 +180,196 @@
{ return new TreelandConfig(thread, nullptr, name, {}, subpath, true, parent); }
static TreelandConfig* createGenericByName(DTK_CORE_NAMESPACE::DConfigBackend *backend, const QString &name, const QString &subpath = {}, QObject *parent = nullptr, QThread *thread = DTK_CORE_NAMESPACE::DConfig::globalThread())
{ return new TreelandConfig(thread, backend, name, {}, subpath, true, parent); }
- ~TreelandConfig() {
- if (m_config.loadRelaxed()) {
- m_config.loadRelaxed()->deleteLater();
- m_config.storeRelaxed(nullptr);
- }
- }
- Q_INVOKABLE DTK_CORE_NAMESPACE::DConfig *config() const {
- return m_config.loadRelaxed();
- }
-
- Q_INVOKABLE bool isInitializeSucceed() const {
- return m_status.loadRelaxed() == static_cast<int>(Status::Succeed);
- }
+ ~TreelandConfig() {
+ int oldStatus = m_data->m_status.fetchAndStoreOrdered(static_cast<int>(Data::Status::Destroyed));
+ m_data->m_userConfig = nullptr;
- Q_INVOKABLE bool isInitializeFailed() const {
- return m_status.loadRelaxed() == static_cast<int>(Status::Failed);
+ if (oldStatus == static_cast<int>(Data::Status::Succeed)) {
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ config->deleteLater();
+ // m_data will be deleted by config->destroyed connection
+ } else {
+ m_data->deleteLater();
+ }
+ } else if (oldStatus == static_cast<int>(Data::Status::Failed) || oldStatus == static_cast<int>(Data::Status::Invalid)) {
+ m_data->deleteLater();
+ }
+ // If Initializing, worker thread handles m_data deletion when it sees Destroyed status
}
- Q_INVOKABLE bool isInitializing() const {
- return m_status.loadRelaxed() == static_cast<int>(Status::Invalid);
- }
+ Q_INVOKABLE DTK_CORE_NAMESPACE::DConfig *config() const { return m_data->m_config.loadRelaxed(); }
+ Q_INVOKABLE bool isInitializeSucceed() const { return m_data->m_status.loadRelaxed() == static_cast<int>(Data::Status::Succeed); }
+ Q_INVOKABLE bool isInitializeFailed() const { return m_data->m_status.loadRelaxed() == static_cast<int>(Data::Status::Failed); }
+ Q_INVOKABLE bool isInitializing() const { return m_data->m_status.loadRelaxed() == static_cast<int>(Data::Status::Initializing); }
- Q_INVOKABLE QStringList keyList() const {
- return { QStringLiteral("enablePrelaunchSplash"),
- QStringLiteral("forceSoftwareCursor"),
- QStringLiteral("prelaunchSplashTimeoutMs")};
- }
+ Q_INVOKABLE QStringList keyList() const { return { QStringLiteral("enablePrelaunchSplash"), QStringLiteral("forceSoftwareCursor"), QStringLiteral("prelaunchSplashTimeoutMs") }; }
Q_INVOKABLE bool isDefaultValue(const QString &key) const {
- if (key == QStringLiteral("enablePrelaunchSplash"))
- return enablePrelaunchSplashIsDefaultValue();
- if (key == QStringLiteral("forceSoftwareCursor"))
- return forceSoftwareCursorIsDefaultValue();
- if (key == QStringLiteral("prelaunchSplashTimeoutMs"))
- return prelaunchSplashTimeoutMsIsDefaultValue();
+ if (key == QStringLiteral("enablePrelaunchSplash")) return enablePrelaunchSplashIsDefaultValue();
+ if (key == QStringLiteral("forceSoftwareCursor")) return forceSoftwareCursorIsDefaultValue();
+ if (key == QStringLiteral("prelaunchSplashTimeoutMs")) return prelaunchSplashTimeoutMsIsDefaultValue();
return false;
}
- bool enablePrelaunchSplash() const {
- return p_enablePrelaunchSplash;
- }
- void setEnablePrelaunchSplash(const bool &value) {
- auto oldValue = p_enablePrelaunchSplash;
- p_enablePrelaunchSplash = value;
- markPropertySet(0);
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this, value]() {
- m_config.loadRelaxed()->setValue(QStringLiteral("enablePrelaunchSplash"), value);
- });
- }
- if (p_enablePrelaunchSplash != oldValue) {
- Q_EMIT enablePrelaunchSplashChanged();
- Q_EMIT valueChanged(QStringLiteral("enablePrelaunchSplash"), value);
+ bool enablePrelaunchSplash() const { return m_data->p_enablePrelaunchSplash; }
+ void setEnablePrelaunchSplash(const bool &v) {
+ if (m_data->p_enablePrelaunchSplash == v && m_data->testPropertySet(0)) return;
+ m_data->p_enablePrelaunchSplash = v;
+ m_data->markPropertySet(0);
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config, v]() { config->setValue(QStringLiteral("enablePrelaunchSplash"), v); });
}
+ Q_EMIT enablePrelaunchSplashChanged();
+ Q_EMIT valueChanged(QStringLiteral("enablePrelaunchSplash"), v);
}
void resetEnablePrelaunchSplash() {
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this]() {
- m_config.loadRelaxed()->reset(QStringLiteral("enablePrelaunchSplash"));
- });
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config]() { config->reset(QStringLiteral("enablePrelaunchSplash")); });
}
}
-#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
- QBindable<bool> bindableEnablePrelaunchSplash() {
- return QBindable<bool>(this, "enablePrelaunchSplash");
- }
-#endif
- Q_INVOKABLE bool enablePrelaunchSplashIsDefaultValue() const {
- return !testPropertySet(0);
- }
- bool forceSoftwareCursor() const {
- return p_forceSoftwareCursor;
- }
- void setForceSoftwareCursor(const bool &value) {
- auto oldValue = p_forceSoftwareCursor;
- p_forceSoftwareCursor = value;
- markPropertySet(1);
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this, value]() {
- m_config.loadRelaxed()->setValue(QStringLiteral("forceSoftwareCursor"), value);
- });
- }
- if (p_forceSoftwareCursor != oldValue) {
- Q_EMIT forceSoftwareCursorChanged();
- Q_EMIT valueChanged(QStringLiteral("forceSoftwareCursor"), value);
+ Q_INVOKABLE bool enablePrelaunchSplashIsDefaultValue() const { return !m_data->testPropertySet(0); }
+ bool forceSoftwareCursor() const { return m_data->p_forceSoftwareCursor; }
+ void setForceSoftwareCursor(const bool &v) {
+ if (m_data->p_forceSoftwareCursor == v && m_data->testPropertySet(1)) return;
+ m_data->p_forceSoftwareCursor = v;
+ m_data->markPropertySet(1);
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config, v]() { config->setValue(QStringLiteral("forceSoftwareCursor"), v); });
}
+ Q_EMIT forceSoftwareCursorChanged();
+ Q_EMIT valueChanged(QStringLiteral("forceSoftwareCursor"), v);
}
void resetForceSoftwareCursor() {
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this]() {
- m_config.loadRelaxed()->reset(QStringLiteral("forceSoftwareCursor"));
- });
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config]() { config->reset(QStringLiteral("forceSoftwareCursor")); });
}
}
-#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
- QBindable<bool> bindableForceSoftwareCursor() {
- return QBindable<bool>(this, "forceSoftwareCursor");
- }
-#endif
- Q_INVOKABLE bool forceSoftwareCursorIsDefaultValue() const {
- return !testPropertySet(1);
- }
- qlonglong prelaunchSplashTimeoutMs() const {
- return p_prelaunchSplashTimeoutMs;
- }
- void setPrelaunchSplashTimeoutMs(const qlonglong &value) {
- auto oldValue = p_prelaunchSplashTimeoutMs;
- p_prelaunchSplashTimeoutMs = value;
- markPropertySet(2);
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this, value]() {
- m_config.loadRelaxed()->setValue(QStringLiteral("prelaunchSplashTimeoutMs"), value);
- });
- }
- if (p_prelaunchSplashTimeoutMs != oldValue) {
- Q_EMIT prelaunchSplashTimeoutMsChanged();
- Q_EMIT valueChanged(QStringLiteral("prelaunchSplashTimeoutMs"), value);
+ Q_INVOKABLE bool forceSoftwareCursorIsDefaultValue() const { return !m_data->testPropertySet(1); }
+ qlonglong prelaunchSplashTimeoutMs() const { return m_data->p_prelaunchSplashTimeoutMs; }
+ void setPrelaunchSplashTimeoutMs(const qlonglong &v) {
+ if (m_data->p_prelaunchSplashTimeoutMs == v && m_data->testPropertySet(2)) return;
+ m_data->p_prelaunchSplashTimeoutMs = v;
+ m_data->markPropertySet(2);
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config, v]() { config->setValue(QStringLiteral("prelaunchSplashTimeoutMs"), v); });
}
+ Q_EMIT prelaunchSplashTimeoutMsChanged();
+ Q_EMIT valueChanged(QStringLiteral("prelaunchSplashTimeoutMs"), v);
}
void resetPrelaunchSplashTimeoutMs() {
- if (auto config = m_config.loadRelaxed()) {
- QMetaObject::invokeMethod(config, [this]() {
- m_config.loadRelaxed()->reset(QStringLiteral("prelaunchSplashTimeoutMs"));
- });
+ if (auto config = m_data->m_config.loadRelaxed()) {
+ QMetaObject::invokeMethod(config, [config]() { config->reset(QStringLiteral("prelaunchSplashTimeoutMs")); });
}
}
-#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
- QBindable<qlonglong> bindablePrelaunchSplashTimeoutMs() {
- return QBindable<qlonglong>(this, "prelaunchSplashTimeoutMs");
- }
-#endif
- Q_INVOKABLE bool prelaunchSplashTimeoutMsIsDefaultValue() const {
- return !testPropertySet(2);
- }
+ Q_INVOKABLE bool prelaunchSplashTimeoutMsIsDefaultValue() const { return !m_data->testPropertySet(2); }
+
Q_SIGNALS:
- void configInitializeFailed(DTK_CORE_NAMESPACE::DConfig *config);
+ void configInitializeFailed();
void configInitializeSucceed(DTK_CORE_NAMESPACE::DConfig *config);
void valueChanged(const QString &key, const QVariant &value);
-
void enablePrelaunchSplashChanged();
void forceSoftwareCursorChanged();
void prelaunchSplashTimeoutMsChanged();
-private:
- void initializeInConfigThread(DTK_CORE_NAMESPACE::DConfig *config) {
- Q_ASSERT(!m_config.loadRelaxed());
- m_config.storeRelaxed(config);
- if (!config->isValid()) {
- m_status.storeRelaxed(static_cast<int>(Status::Failed));
- Q_EMIT configInitializeFailed(config);
- return;
- }
-
- if (testPropertySet(0)) {
- config->setValue(QStringLiteral("enablePrelaunchSplash"), QVariant::fromValue(p_enablePrelaunchSplash));
- } else {
- updateValue(QStringLiteral("enablePrelaunchSplash"), QVariant::fromValue(p_enablePrelaunchSplash));
- }
- if (testPropertySet(1)) {
- config->setValue(QStringLiteral("forceSoftwareCursor"), QVariant::fromValue(p_forceSoftwareCursor));
- } else {
- updateValue(QStringLiteral("forceSoftwareCursor"), QVariant::fromValue(p_forceSoftwareCursor));
- }
- if (testPropertySet(2)) {
- config->setValue(QStringLiteral("prelaunchSplashTimeoutMs"), QVariant::fromValue(p_prelaunchSplashTimeoutMs));
- } else {
- updateValue(QStringLiteral("prelaunchSplashTimeoutMs"), QVariant::fromValue(p_prelaunchSplashTimeoutMs));
- }
- if (!m_config.loadRelaxed())
- return;
- connect(config, &DTK_CORE_NAMESPACE::DConfig::valueChanged, this, [this](const QString &key) {
- updateValue(key);
- }, Qt::DirectConnection);
+private:
+ Data *m_data;
+};
- m_status.storeRelaxed(static_cast<int>(Status::Succeed));
- Q_EMIT configInitializeSucceed(config);
+inline void TreelandConfigData::initializeInConfigThread(DTK_CORE_NAMESPACE::DConfig *config) {
+ Q_ASSERT(!m_config.loadRelaxed());
+ m_config.storeRelaxed(config);
+ if (testPropertySet(0)) config->setValue(QStringLiteral("enablePrelaunchSplash"), QVariant::fromValue(p_enablePrelaunchSplash));
+ else updateValue(QStringLiteral("enablePrelaunchSplash"), QVariant::fromValue(p_enablePrelaunchSplash));
+ if (testPropertySet(1)) config->setValue(QStringLiteral("forceSoftwareCursor"), QVariant::fromValue(p_forceSoftwareCursor));
+ else updateValue(QStringLiteral("forceSoftwareCursor"), QVariant::fromValue(p_forceSoftwareCursor));
+ if (testPropertySet(2)) config->setValue(QStringLiteral("prelaunchSplashTimeoutMs"), QVariant::fromValue(p_prelaunchSplashTimeoutMs));
+ else updateValue(QStringLiteral("prelaunchSplashTimeoutMs"), QVariant::fromValue(p_prelaunchSplashTimeoutMs));
+ // Transition from Initializing to Succeed
+ if (!m_status.testAndSetOrdered(static_cast<int>(Status::Initializing), static_cast<int>(Status::Succeed))) {
+ if (m_status.loadRelaxed() == static_cast<int>(Status::Destroyed)) {
+ config->deleteLater();
+ deleteLater();
+ }
+ return;
+ }}
+
+inline void TreelandConfigData::updateValue(const QString &key, const QVariant &fallback) {
+ auto config = m_config.loadRelaxed();
+ if (!config) return;
+ const QVariant &v = config->value(key, fallback);
+ if (key == QStringLiteral("enablePrelaunchSplash")) {
+ markPropertySet(0, !config->isDefaultValue(key));
+ // Safe capture using QPointer to handle object destruction
+ QPointer<TreelandConfigData> safeThis(this);
+ QMetaObject::invokeMethod(this, [safeThis, key, v]() {
+ if (safeThis) safeThis->updateProperty(key, v);
+ });
+ return;
}
- void updateValue(const QString &key, const QVariant &fallback = QVariant()) {
- if (!m_config.loadRelaxed())
- return;
- Q_ASSERT(QThread::currentThread() == m_config.loadRelaxed()->thread());
- const QVariant &value = m_config.loadRelaxed()->value(key, fallback);
- if (key == QStringLiteral("enablePrelaunchSplash")) {
- markPropertySet(0, !m_config.loadRelaxed()->isDefaultValue(key));
- auto newValue = qvariant_cast<bool>(value);
- QMetaObject::invokeMethod(this, [this, newValue, key, value]() {
- Q_ASSERT(QThread::currentThread() == this->thread());
- if (p_enablePrelaunchSplash != newValue) {
- p_enablePrelaunchSplash = newValue;
- Q_EMIT enablePrelaunchSplashChanged();
- Q_EMIT valueChanged(key, value);
- }
- });
- return;
- }
- if (key == QStringLiteral("forceSoftwareCursor")) {
- markPropertySet(1, !m_config.loadRelaxed()->isDefaultValue(key));
- auto newValue = qvariant_cast<bool>(value);
- QMetaObject::invokeMethod(this, [this, newValue, key, value]() {
- Q_ASSERT(QThread::currentThread() == this->thread());
- if (p_forceSoftwareCursor != newValue) {
- p_forceSoftwareCursor = newValue;
- Q_EMIT forceSoftwareCursorChanged();
- Q_EMIT valueChanged(key, value);
- }
- });
- return;
- }
- if (key == QStringLiteral("prelaunchSplashTimeoutMs")) {
- markPropertySet(2, !m_config.loadRelaxed()->isDefaultValue(key));
- auto newValue = qvariant_cast<qlonglong>(value);
- QMetaObject::invokeMethod(this, [this, newValue, key, value]() {
- Q_ASSERT(QThread::currentThread() == this->thread());
- if (p_prelaunchSplashTimeoutMs != newValue) {
- p_prelaunchSplashTimeoutMs = newValue;
- Q_EMIT prelaunchSplashTimeoutMsChanged();
- Q_EMIT valueChanged(key, value);
+ if (key == QStringLiteral("forceSoftwareCursor")) {
+ markPropertySet(1, !config->isDefaultValue(key));
+ // Safe capture using QPointer to handle object destruction
+ QPointer<TreelandConfigData> safeThis(this);
+ QMetaObject::invokeMethod(this, [safeThis, key, v]() {
+ if (safeThis) safeThis->updateProperty(key, v);
+ });
+ return;
+ }
+ if (key == QStringLiteral("prelaunchSplashTimeoutMs")) {
+ markPropertySet(2, !config->isDefaultValue(key));
+ // Safe capture using QPointer to handle object destruction
+ QPointer<TreelandConfigData> safeThis(this);
+ QMetaObject::invokeMethod(this, [safeThis, key, v]() {
+ if (safeThis) safeThis->updateProperty(key, v);
+ });
+ return;
+ }
+}
+
+inline void TreelandConfigData::updateProperty(const QString &key, const QVariant &v) {
+ if (key == QStringLiteral("enablePrelaunchSplash")) {
+ bool nv = qvariant_cast<bool>(v);
+ if (p_enablePrelaunchSplash != nv) {
+ p_enablePrelaunchSplash = nv;
+ if (m_userConfig) {
+ auto uc = m_userConfig;
+ if (uc) {
+ Q_EMIT uc.data()->enablePrelaunchSplashChanged();
+ Q_EMIT uc.data()->valueChanged(key, v);
}
- });
- return;
+ }
}
+ return;
}
- inline void markPropertySet(const int index, bool on = true) {
- if (index < 32) {
- if (on)
- m_propertySetStatus0.fetchAndOrOrdered(1 << (index - 0));
- else
- m_propertySetStatus0.fetchAndAndOrdered(~(1 << (index - 0)));
- return;
+ if (key == QStringLiteral("forceSoftwareCursor")) {
+ bool nv = qvariant_cast<bool>(v);
+ if (p_forceSoftwareCursor != nv) {
+ p_forceSoftwareCursor = nv;
+ if (m_userConfig) {
+ auto uc = m_userConfig;
+ if (uc) {
+ Q_EMIT uc.data()->forceSoftwareCursorChanged();
+ Q_EMIT uc.data()->valueChanged(key, v);
+ }
+ }
}
- Q_UNREACHABLE();
+ return;
}
- inline bool testPropertySet(const int index) const {
- if (index < 32) {
- return (m_propertySetStatus0.loadRelaxed() & (1 << (index - 0)));
+ if (key == QStringLiteral("prelaunchSplashTimeoutMs")) {
+ qlonglong nv = qvariant_cast<qlonglong>(v);
+ if (p_prelaunchSplashTimeoutMs != nv) {
+ p_prelaunchSplashTimeoutMs = nv;
+ if (m_userConfig) {
+ auto uc = m_userConfig;
+ if (uc) {
+ Q_EMIT uc.data()->prelaunchSplashTimeoutMsChanged();
+ Q_EMIT uc.data()->valueChanged(key, v);
+ }
+ }
}
- Q_UNREACHABLE();
+ return;
}
+}
- QAtomicPointer<DTK_CORE_NAMESPACE::DConfig> m_config = nullptr;
-
-public:
- enum class Status {
- Invalid = 0,
- Succeed = 1,
- Failed = 2
- };
-private:
- QAtomicInteger<int> m_status = static_cast<int>(Status::Invalid);
-
- bool p_enablePrelaunchSplash { false };
- bool p_forceSoftwareCursor { false };
- qlonglong p_prelaunchSplashTimeoutMs { 5000 };
- QAtomicInteger<quint32> m_propertySetStatus0 = 0;
-};
-
-#endif // TREELANDCONFIG_H
+#endif
|
Contributor
deepin pr auto review我来对这个diff进行代码审查：
总的来说，这次改动显著提升了代码的质量和安全性，是一个很好的重构。主要改进包括更好的线程安全性、更清晰的代码结构以及更完善的错误处理机制。 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the DConfig C++ wrapper class generation tool (dconfig2cpp) to improve thread safety and lifecycle management. The refactor introduces a data layer separation pattern, enhances the state machine from 3 states to 5 states, and implements atomic operations for thread-safe state transitions.
Key changes:
- Introduces a Data class (TreelandUserConfigData) to separate internal data management from the public API (TreelandUserConfig)
- Expands state machine from Invalid/Succeed/Failed to Invalid/Initializing/Succeed/Failed/Destroyed with atomic CAS operations
- Separates thread responsibilities: updateValue() in worker thread, updateProperty() in main thread with QMetaObject::invokeMethod for proper thread context
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| << " enum class Status {\n" | ||
| << " Invalid = 0,\n" | ||
| << " Initializing = 1,\n" | ||
| << " Succeed = 2,\n" |
There was a problem hiding this comment.
Grammar issue in enum value: 'Succeed' should be 'Succeeded' to follow standard English grammar. While this is internal to the Data class and doesn't affect the public API (which uses 'isInitializeSucceed' for backward compatibility), it would be better to use correct grammar for the enum values to maintain code clarity.
Suggested change
| << " Succeed = 2,\n" | |
| << " Succeeded = 2,\n" |
Comment on lines
+625
to
+630
| << " if (m_userConfig) {\n" | ||
| << " auto uc = m_userConfig;\n" | ||
| << " if (uc) {\n" | ||
| << " Q_EMIT uc.data()->" << p.propertyName << "Changed();\n" | ||
| << " Q_EMIT uc.data()->valueChanged(key, v);\n" | ||
| << " }\n" |
There was a problem hiding this comment.
Race condition in signal emission: The code checks m_userConfig, captures it to 'uc', then checks 'uc' again. However, between capturing to 'uc' and calling 'uc.data()', the object could be destroyed, making uc null. The pattern should be: capture once, check once, then use the captured value. Current code: 'if (m_userConfig) { auto uc = m_userConfig; if (uc) { uc.data()->...' should be: 'auto uc = m_userConfig; if (uc) { uc->...' (note: QPointer has operator-> that does the null check internally).
Suggested change
| << " if (m_userConfig) {\n" | |
| << " auto uc = m_userConfig;\n" | |
| << " if (uc) {\n" | |
| << " Q_EMIT uc.data()->" << p.propertyName << "Changed();\n" | |
| << " Q_EMIT uc.data()->valueChanged(key, v);\n" | |
| << " }\n" | |
| << " auto uc = m_userConfig;\n" | |
| << " if (uc) {\n" | |
| << " Q_EMIT uc->" << p.propertyName << "Changed();\n" | |
| << " Q_EMIT uc->valueChanged(key, v);\n" |
Comment on lines
+553
to
+555
| << " if (auto config = m_data->m_config.loadRelaxed()) {\n" | ||
| << " QMetaObject::invokeMethod(config, [config, v]() { config->setValue(" << p.propertyNameString | ||
| << ", v); });\n" |
There was a problem hiding this comment.
Lambda captures 'config' by value but config is a raw pointer loaded from an atomic. If the config is deleted on another thread (e.g., destructor runs) after the loadRelaxed() call but before the lambda executes, this becomes a use-after-free. The lambda should capture config in a way that ensures its lifetime, or re-check the atomic pointer inside the lambda.
Comment on lines
+593
to
+600
| headerStream << R"( // Transition from Initializing to Succeed | ||
| if (!m_status.testAndSetOrdered(static_cast<int>(Status::Initializing), static_cast<int>(Status::Succeed))) { | ||
| if (m_status.loadRelaxed() == static_cast<int>(Status::Destroyed)) { | ||
| config->deleteLater(); | ||
| deleteLater(); | ||
| } | ||
| headerStream << " " << property.typeName << " p_" << property.propertyName << " { "; | ||
| headerStream << jsonValueToCppCode(property.defaultValue) << " };\n"; | ||
| return; | ||
| }} |
There was a problem hiding this comment.
Potential race condition in state transition check: After failing the testAndSetOrdered at line 594, the code checks if status is Destroyed at line 595. However, the status could have transitioned to a different state (e.g., another thread could change it). The non-atomic check after the CAS operation doesn't provide the same guarantees. Consider using testAndSetOrdered in a loop or handling all possible state transitions explicitly after the failed CAS.
Comment on lines
+402
to
+405
| QMetaObject::invokeMethod(data, [data]() { | ||
| if (data->m_userConfig) | ||
| Q_EMIT data->m_userConfig->configInitializeFailed(); | ||
| }); |
There was a problem hiding this comment.
Race condition: The QPointer m_userConfig is checked without atomic protection. Between checking 'if (data->m_userConfig)' and emitting the signal, the main thread destructor could set m_userConfig to nullptr, leading to use-after-free when accessing m_userConfig->configInitializeFailed(). Consider using atomic operations or checking the QPointer twice (once to capture, then check the captured value).
| << " Q_EMIT valueChanged(" << property.propertyNameString << ", value);\n" | ||
| const auto &p = properties[i]; | ||
| const QString read = usedKeywords.contains(p.propertyName) ? "get" + p.capitalizedPropertyName : p.propertyName; | ||
| headerStream << " " << p.typeName << " " << read << "() const { return m_data->p_" << p.propertyName << "; }\n" |
There was a problem hiding this comment.
Thread safety issue: The property value is read from m_data without synchronization. While atomic operations protect status and config pointer, the property data itself (p_propertyName) is not protected. This creates a race between the main thread reading the property and the worker thread updating it via updateProperty, potentially returning torn or inconsistent values.
Comment on lines
+561
to
+563
| << " if (auto config = m_data->m_config.loadRelaxed()) {\n" | ||
| << " QMetaObject::invokeMethod(config, [config]() { config->reset(" << p.propertyNameString | ||
| << "); });\n" |
There was a problem hiding this comment.
Same use-after-free issue as in setter: config is captured by value as a raw pointer. If the destructor deletes config between loadRelaxed() and lambda execution, this will access freed memory. Consider using QPointer or re-checking the atomic pointer inside the lambda.
| // Initialize through Data class | ||
| data->initializeInConfigThread(config); | ||
| QObject::connect(config, &DTK_CORE_NAMESPACE::DConfig::valueChanged, data, [data](const QString &key) { data->updateValue(key); }, Qt::DirectConnection); |
There was a problem hiding this comment.
Potential use-after-free in valueChanged connection: The lambda captures 'data' by value (raw pointer), but if the destructor is called and deletes m_data while the config thread is processing value changes, the callback will access freed memory. Since the config object's lifetime can extend beyond m_data (config is deleted with deleteLater which is asynchronous), this connection should use QPointer or a similar mechanism to safely check if data is still valid.
Comment on lines
+365
to
+369
| if (!data->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid), | ||
| static_cast<int>(Data::Status::Initializing))) { | ||
| worker->deleteLater(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing cleanup after failed state transition: If the CAS operation fails at line 365 (status is not Invalid), the worker is deleted but m_data is never cleaned up. This could happen if the constructor is called multiple times or if there's a bug elsewhere that changes the status. The code should explicitly handle this case by checking what state it transitioned from and cleaning up m_data appropriately, or at minimum add a comment explaining why this is safe.
Comment on lines
+550
to
+558
| << " if (m_data->p_" << p.propertyName << " == v && m_data->testPropertySet(" << i << ")) return;\n" | ||
| << " m_data->p_" << p.propertyName << " = v;\n" | ||
| << " m_data->markPropertySet(" << i << ");\n" | ||
| << " if (auto config = m_data->m_config.loadRelaxed()) {\n" | ||
| << " QMetaObject::invokeMethod(config, [config, v]() { config->setValue(" << p.propertyNameString | ||
| << ", v); });\n" | ||
| << " }\n" | ||
| << " Q_EMIT " << p.propertyName << "Changed();\n" | ||
| << " Q_EMIT valueChanged(" << p.propertyNameString << ", v);\n" |
There was a problem hiding this comment.
Signals emitted even when value doesn't change: When a property is set for the first time to a value that equals its default value, the setter will emit Changed signals even though the actual value hasn't changed (testPropertySet returns false, so the early return is skipped). This could lead to unnecessary updates and confusion. Consider tracking whether the value actually changed before emitting signals, or document this behavior clearly.
Suggested change
| << " if (m_data->p_" << p.propertyName << " == v && m_data->testPropertySet(" << i << ")) return;\n" | |
| << " m_data->p_" << p.propertyName << " = v;\n" | |
| << " m_data->markPropertySet(" << i << ");\n" | |
| << " if (auto config = m_data->m_config.loadRelaxed()) {\n" | |
| << " QMetaObject::invokeMethod(config, [config, v]() { config->setValue(" << p.propertyNameString | |
| << ", v); });\n" | |
| << " }\n" | |
| << " Q_EMIT " << p.propertyName << "Changed();\n" | |
| << " Q_EMIT valueChanged(" << p.propertyNameString << ", v);\n" | |
| << " bool __dconfig2cppValueChanged = (m_data->p_" << p.propertyName << " != v);\n" | |
| << " if (!__dconfig2cppValueChanged && m_data->testPropertySet(" << i << ")) return;\n" | |
| << " m_data->p_" << p.propertyName << " = v;\n" | |
| << " m_data->markPropertySet(" << i << ");\n" | |
| << " if (auto config = m_data->m_config.loadRelaxed()) {\n" | |
| << " QMetaObject::invokeMethod(config, [config, v]() { config->setValue(" << p.propertyNameString | |
| << ", v); });\n" | |
| << " }\n" | |
| << " if (__dconfig2cppValueChanged) {\n" | |
| << " Q_EMIT " << p.propertyName << "Changed();\n" | |
| << " Q_EMIT valueChanged(" << p.propertyNameString << ", v);\n" | |
| << " }\n" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…lifecycle management
Problem:
Solution:
Introduce Data layer separation (TreelandUserConfigData + TreelandUserConfig)
Enhance state machine (3-state -> 5-state model)
Improve async initialization and cleanup
Separate thread responsibilities
Improvements: