fix: Refactor DConfig wrapper class generation for thread safety and …

[Dami-star]

…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

[deepin-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

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.

[zccrs]

还没有看完

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[deepin-ci-robot]

deepin pr auto review

代码审查报告

总体评价

这是一个对DConfig配置管理相关代码的更新，主要改进了线程安全性和对象生命周期管理。代码引入了新的Data类来封装配置数据，并使用原子操作和状态机来管理配置的初始化过程。

主要改进点

1. 线程安全性增强

   • 引入了Data类封装配置数据，通过QPointer和原子操作确保线程安全
   • 使用testAndSetOrdered实现原子状态转换
   • 改进了对象生命周期管理，防止竞态条件

2. 对象生命周期管理

   • 实现了清晰的状态机(Invalid→Initializing→Succeeded/Failed→Destroyed)
   • 使用QObject::destroyed信号自动清理Data对象
   • 在析构函数中正确处理不同状态下的资源释放

3. 新增功能

   • 添加了colorMode和defaultColorMode两个新属性
   • 添加了configInitializeFailed()信号(不带参数)
   • 添加了valueChanged信号通知配置变更
   • 添加了reset方法用于重置配置值

4. 代码组织改进

   • 将配置数据封装到Data类中，提高了代码组织性
   • 添加了event方法重写，处理线程变更事件
   • 使用using QObject::moveToThread防止外部调用

潜在问题与建议

1. 线程安全与竞态条件

问题：在updateValue方法中，虽然检查了m_userConfig是否有效，但在多线程环境下，m_userConfig可能在检查后被设置为nullptr。

QMetaObject::invokeMethod(this, [this, newValue, key, value]() {
    if (m_userConfig && p_autoDisplayFeature != newValue) {
        // m_userConfig可能在这里被设置为nullptr
        Q_ASSERT(QThread::currentThread() == m_userConfig->thread());
        // ...
    }
});

建议：在lambda捕获时使用QPointer捕获m_userConfig，确保其有效性：

QPointer<dconfig_org_deepin_dtk_preference> safeUserConfig(m_userConfig);
QMetaObject::invokeMethod(this, [safeUserConfig, newValue, key, value]() {
    if (safeUserConfig && safeUserConfig->m_data && safeUserConfig->m_data->p_autoDisplayFeature != newValue) {
        Q_ASSERT(QThread::currentThread() == safeUserConfig->thread());
        safeUserConfig->m_data->p_autoDisplayFeature = newValue;
        Q_EMIT safeUserConfig->autoDisplayFeatureChanged();
        Q_EMIT safeUserConfig->valueChanged(key, value);
    }
});

2. 错误处理改进

问题：当配置初始化失败时，configInitializeFailed()信号不带参数，调用者无法获取失败原因。

建议：考虑添加错误码或错误消息参数：

Q_SIGNALS:
    void configInitializeFailed(const QString &errorMessage);

3. 内存管理优化

问题：在构造函数中，worker对象在lambda内被删除，但lambda本身可能还未执行完。

QMetaObject::invokeMethod(worker, [safeData, backend, name, appId, subpath, isGeneric, worker]() mutable {
    delete worker;
    worker = nullptr;
    // ...
});

建议：使用QPointer管理worker对象，确保安全删除：

QPointer<QObject> safeWorker(worker);
QMetaObject::invokeMethod(worker, [safeData, backend, name, appId, subpath, isGeneric, safeWorker]() mutable {
    if (safeWorker) {
        safeWorker->deleteLater();
    }
    // ...
});

4. 性能优化

问题：每次属性变更都会触发两个信号(xxxChanged和valueChanged)，可能造成不必要的开销。

建议：考虑提供选项让用户选择是否需要valueChanged信号，或者只在特定条件下触发。

5. 代码可读性

问题：状态转换逻辑较为复杂，特别是在构造函数和析构函数中。

建议：将状态转换逻辑封装为独立方法，提高可读性：

bool tryTransitionToInitializing() {
    return m_status.testAndSetOrdered(static_cast<int>(Status::Invalid),
                                      static_cast<int>(Status::Initializing));
}

bool tryTransitionToSucceeded() {
    return m_status.testAndSetOrdered(static_cast<int>(Status::Initializing),
                                      static_cast<int>(Status::Succeeded));
}

bool tryTransitionToFailed() {
    return m_status.testAndSetOrdered(static_cast<int>(Status::Initializing),
                                      static_cast<int>(Status::Failed));
}

6. 测试覆盖

建议：确保以下场景有充分的单元测试覆盖：

1. 多线程环境下的并发访问
2. 快速创建和销毁对象
3. 配置初始化失败场景
4. 属性变更通知
5. 状态转换的边界条件

其他建议

1. 文档完善：为新增的Data类和状态机添加详细注释，说明各状态之间的转换条件和时机。

2. 日志记录：在关键状态转换点添加调试日志，便于问题诊断。

3. 异常安全：考虑添加异常处理机制，防止配置操作中的异常导致程序崩溃。

4. 兼容性：确保新版本与旧版本API的兼容性，或者提供迁移指南。

总结

这次更新显著提高了代码的线程安全性和对象生命周期管理，但仍有改进空间。主要关注点应放在多线程环境下的竞态条件处理和错误处理机制上。建议在合并前进行充分的测试，特别是多线程场景下的压力测试。

[zccrs]

在修改m_status之前赋值

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The Status enum naming is inconsistent. The new state uses "Succeeded" (line 529) but the old comparison in isInitializeSucceed uses "Succeed" pattern (line 442). While this is just a rename within the same PR, the function name isInitializeSucceed should be isInitializeSucceeded for grammatical consistency with the enum value name.

Suggested change

	Q_INVOKABLE bool isInitializeSucceed() const {
	Q_INVOKABLE bool isInitializeSucceeded() const {

[Copilot]

When the CAS operation fails at line 302-304, safeData->deleteLater() is called. However, this could cause a double-delete issue. If the CAS fails because the destructor already set the state to Destroyed, the destructor is responsible for cleaning up Data. Calling deleteLater() here could attempt to delete an object that's already being destroyed. Consider checking the actual state value after CAS failure to determine the appropriate action.

Suggested change

	if (!safeData->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid),
	static_cast<int>(Data::Status::Initializing))) {
	// CAS failed, state already changed - userConfig destructor will handle cleanup
	// Do not attempt to delete here as it would race with destructor
	safeData->deleteLater();
	if (safeData && !safeData->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid),
	static_cast<int>(Data::Status::Initializing))) {
	// CAS failed, state already changed.
	// If the state is Destroyed, the destructor is responsible for cleanup.
	const int currentStatus = safeData->m_status.loadAcquire();
	if (currentStatus != static_cast<int>(Data::Status::Destroyed)) {
	// Only schedule deletion if the object is still alive and not in Destroyed state.
	safeData->deleteLater();
	}