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.

[Copilot]

The assertion on line 585 'Q_ASSERT(QThread::currentThread() == selfData->m_userConfig->thread())' may dereference a null pointer. If selfData is valid but m_userConfig has been set to nullptr by the destructor (line 428), this will crash. The check on line 586 'if (selfData && selfData->m_userConfig)' should come before the assertion.

Suggested change

	<< " Q_ASSERT(QThread::currentThread() == selfData->m_userConfig->thread());\n"
	<< " if (selfData && selfData->m_userConfig) {\n"
	<< " if (selfData && selfData->m_userConfig) {\n"
	<< " Q_ASSERT(QThread::currentThread() == selfData->m_userConfig->thread());\n"

[Copilot]

The signal 'configInitializeFailed' has changed its signature from 'configInitializeFailed(DTK_CORE_NAMESPACE::DConfig *config)' to 'configInitializeFailed()' without the config parameter. This is a breaking API change that will cause compilation errors for any code connecting to this signal with the old signature.

[Copilot]

The code at line 435 claims that m_data will be deleted by config's destroyed signal when status is Succeeded. However, if the config object is deleted before the signal connection is fully established (between lines 369-373), the destroyed signal won't trigger the deletion of m_data, causing a memory leak. Consider using a connection guard or ensuring the connection happens synchronously.

Suggested change

	// When Succeeded, release config object only
	auto config = m_data->m_config.loadRelaxed();
	Q_ASSERT(config);
	config->deleteLater();
	// m_data will be deleted by config's destroyed signal
	// When Succeeded, release config object and ensure Data is also cleaned up
	auto config = m_data->m_config.loadRelaxed();
	Q_ASSERT(config);
	config->deleteLater();
	m_data->deleteLater();

[Copilot]

The assertion at line 432 assumes that config is always non-null when status is Succeeded. However, there's a potential race condition: if m_data is deleted (and status set to Destroyed) between checking oldStatus and loading the config pointer, the config could be null even when oldStatus was Succeeded. The code should check if config is null before asserting.

Suggested change

	Q_ASSERT(config);
	config->deleteLater();
	if (config) {
	config->deleteLater();
	}

[deepin-ci-robot]

deepin pr auto review

这份代码是一个由dconfig2cpp工具自动生成的配置管理类代码，主要涉及Deepin DTK框架中的配置管理功能。我将对代码进行全面的审查，包括语法逻辑、代码质量、性能和安全性等方面。

语法逻辑审查

1. 类结构变化

代码引入了一个新的内部类Data，将原来直接在主类中定义的成员变量和方法封装到了这个内部类中。这是一个良好的设计改进，有助于：

• 分离关注点
• 提高代码可维护性
• 更好地管理资源生命周期

2. 状态管理

新增了状态枚举和状态管理机制：

enum class Status {
    Invalid = 0,
    Initializing = 1,
    Succeeded = 2,
    Failed = 3,
    Destroyed = 4
};

这种状态机设计更加完善，可以清晰地跟踪配置初始化过程，避免了之前代码中可能存在的状态不一致问题。

3. 线程安全改进

代码中使用了原子操作和CAS(Compare-And-Swap)来确保线程安全：

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
    return;
}

这是一个很好的改进，可以避免竞态条件。

代码质量审查

1. 优点

• 代码结构更加清晰，职责分离明确
• 添加了更多状态检查和错误处理
• 使用了智能指针(QPointer)来管理对象生命周期
• 添加了更多的注释，提高了代码可读性

2. 可改进之处

2.1 重复代码过多

代码中存在大量重复的模式，例如：

if (key == QStringLiteral("autoDisplayFeature")) {
    markPropertySet(0, !m_config.loadRelaxed()->isDefaultValue(key));
    auto newValue = qvariant_cast<bool>(value);
    QMetaObject::invokeMethod(this, [this, newValue, key, value]() {
        Q_ASSERT(QThread::currentThread() == m_userConfig->thread());
        if (m_userConfig && p_autoDisplayFeature != newValue) {
            p_autoDisplayFeature = newValue;
            Q_EMIT m_userConfig->autoDisplayFeatureChanged();
            Q_EMIT m_userConfig->valueChanged(key, value);
        }
    });
    return;
}

这种模式在代码中重复出现多次，可以考虑使用模板或宏来减少重复代码。

2.2 魔法数字

代码中使用了硬编码的数字来表示属性索引：

markPropertySet(0, !m_config.loadRelaxed()->isDefaultValue(key));

建议使用枚举或常量来替代这些魔法数字，提高代码可读性。

2.3 过长的lambda表达式

代码中存在一些过长的lambda表达式，例如：

QMetaObject::invokeMethod(worker, [safeData, backend, name, appId, subpath, isGeneric, worker]() mutable {
    // 大量代码...
});

建议将这些lambda表达式提取为独立的成员函数，提高代码可读性和可维护性。

代码性能审查

1. 优点

• 使用了原子操作来保证线程安全，避免了锁的开销
• 使用了QMetaObject::invokeMethod进行跨线程调用，符合Qt的线程安全机制

2. 可改进之处

2.1 频繁的字符串比较

代码中频繁使用字符串比较来识别属性：

if (key == QStringLiteral("autoDisplayFeature")) {
    // ...
}
if (key == QStringLiteral("colorMode")) {
    // ...
}

这种线性查找方式在属性数量较多时会影响性能。建议使用哈希表或枚举来提高查找效率。

2.2 不必要的 QVariant 转换

代码中频繁进行 QVariant 转换：

auto newValue = qvariant_cast<bool>(value);

如果属性类型已知，可以考虑直接存储和访问，避免不必要的转换开销。

代码安全审查

1. 优点

• 使用了QPointer来管理对象生命周期，避免悬空指针
• 添加了状态检查，避免在对象销毁后访问
• 使用了原子操作来保证线程安全

2. 可改进之处

2.1 潜在的内存泄漏风险

在构造函数中：

auto worker = new QObject();
worker->moveToThread(thread);

如果后续操作失败，worker对象可能不会被正确释放。建议使用QScopedPointer或智能指针来管理worker对象的生命周期。

2.2 潜在的竞态条件

在析构函数中：

~dconfig_org_deepin_dtk_preference() {
    int oldStatus = m_data->m_status.fetchAndStoreOrdered(static_cast<int>(Data::Status::Destroyed));
    m_data->m_userConfig = nullptr;
    // ...
}

虽然使用了原子操作，但在设置状态和清空指针之间仍可能存在竞态条件。建议使用更严格的同步机制。

2.3 缺少边界检查

在属性设置函数中：

void setAutoDisplayFeature(const bool &value) {
    auto oldValue = m_data->p_autoDisplayFeature;
    m_data->p_autoDisplayFeature = value;
    m_data->markPropertySet(0);
    // ...
}

没有对输入值进行验证，如果属性有合法范围限制，应该添加边界检查。

改进建议

1. 代码结构改进

• 考虑使用模板或宏来减少重复代码
• 将过长的lambda表达式提取为独立函数
• 使用枚举或常量替代魔法数字

2. 性能优化

• 使用哈希表或枚举来优化属性查找
• 减少不必要的 QVariant 转换
• 考虑使用更高效的数据结构

3. 安全性增强

• 使用智能指针管理对象生命周期
• 添加更严格的同步机制
• 对输入值进行验证和边界检查

4. 代码可读性提升

• 添加更多注释说明复杂逻辑
• 使用更具描述性的变量名
• 将复杂函数拆分为更小的函数

总结

这份代码总体上是一个良好的改进，引入了更好的状态管理和线程安全机制。但仍有一些可以改进的地方，主要集中在减少重复代码、提高性能和增强安全性方面。建议在后续版本中逐步实现这些改进，以提高代码质量和可维护性。

由于这是自动生成的代码，建议在生成工具(dconfig2cpp)中进行这些改进，而不是手动修改生成的代码，以确保代码的一致性和可维护性。