Skip to content

Conversation

@Dami-star
Copy link

…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
Copy link
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.

Details Needs approval from an approver in each of these files:

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

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
@wineee wineee requested a review from Copilot January 7, 2026 09:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@zccrs zccrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

还没有看完

deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 13, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 13, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
@Dami-star Dami-star requested a review from zccrs January 14, 2026 01:52
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 14, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
@zccrs zccrs requested a review from Copilot January 14, 2026 06:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m_data->deleteLater();
}
} else if (oldStatus == static_cast<int>(Data::Status::Failed) || oldStatus == static_cast<int>(Data::Status::Invalid)) {
m_data->deleteLater();
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The destructor has a potential memory leak scenario. When oldStatus is Initializing, m_data is not deleted. This can occur if the destructor is called while initialization is still in progress on the worker thread. The code should handle the Initializing state by marking it as Destroyed and allowing the worker thread to clean up, or explicitly delete m_data.

Suggested change
m_data->deleteLater();
m_data->deleteLater();
} else if (oldStatus == static_cast<int>(Data::Status::Initializing)) {
m_data->deleteLater();

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

当 oldStatus 为 Initializing 时,我们不会在此处删除, 若在此处删除会导致空指针。工作线程将检测到 Destroyed 状态并自行清理 m_data。

deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 14, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 15, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 15, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 15, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 16, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
deepin-ci-robot added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 16, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
…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 added a commit to linuxdeepin/dtk6core that referenced this pull request Jan 16, 2026
Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#531
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

代码审查报告

总体评估

这段代码是自动生成的 DTK (Deepin Tool Kit) 配置管理类代码,主要变更涉及线程安全改进、内存管理优化和代码结构重构。整体上代码质量有显著提升,但仍存在一些可以改进的地方。

具体改进建议

1. 线程安全改进

优点

  • 引入了 Data 内部类来封装配置数据,改善了数据封装性
  • 使用 QAtomicPointerQAtomicInteger 实现原子操作
  • 使用 QPointer 保护跨线程访问的对象生命周期

建议

// 在 Data 类中添加线程安全保护
class Data : public QObject {
public:
    // 使用读写锁保护复杂类型的数据访问
    mutable QReadWriteLock m_dataLock;
    
    // 修改属性访问方法
    QString colorMode() const {
        QReadLocker locker(&m_dataLock);
        return p_colorMode;
    }
    
    void setColorMode(const QString &v) {
        QWriteLocker locker(&m_dataLock);
        if (p_colorMode == v && testPropertySet(1))
            return;
        p_colorMode = v;
        markPropertySet(1);
        // ... 其余代码
    }
};

2. 内存管理改进

优点

  • 使用 QPointer 避免悬空指针
  • 改进了析构函数中的资源释放逻辑

建议

// 在析构函数中添加更明确的资源清理
~dconfig_org_deepin_dtk_preference() {
    int oldStatus = m_data->m_status.fetchAndStoreOrdered(static_cast<int>(Data::Status::Destroyed));
    
    if (oldStatus == static_cast<int>(Data::Status::Succeeded)) {
        auto config = m_data->m_config.loadRelaxed();
        if (config) {
            // 断开所有信号连接
            QObject::disconnect(config, nullptr, m_data, nullptr);
            config->deleteLater();
        }
        // 使用 QPointer 安全删除
        QPointer<Data> data(m_data);
        if (data) {
            data->deleteLater();
        }
    }
}

3. 错误处理改进

建议

// 在初始化失败时添加更详细的错误信息
if (!config || !config->isValid()) {
    QString errorMsg = QLatin1String("Failed to create DConfig instance.");
    if (config) {
        errorMsg += QLatin1String(" Config exists but is invalid.");
    }
    qWarning() << errorMsg;
    
    // 添加错误代码
    emit initializationError(static_cast<int>(Data::Status::Failed));
    // ... 其余代码
}

4. 代码性能优化

建议

// 减少不必要的 QVariant 转换
void setColorMode(const QString &v) {
    // 使用 const QString& 避免拷贝
    if (m_data->p_colorMode == v && m_data->testPropertySet(1))
        return;
    
    // 使用移动语义
    m_data->p_colorMode = std::move(v);
    // ... 其余代码
}

// 批量更新时减少信号发射
void updateProperties(const QMap<QString, QVariant>& updates) {
    bool changed = false;
    for (auto it = updates.begin(); it != updates.end(); ++it) {
        if (updatePropertyInternal(it.key(), it.value())) {
            changed = true;
        }
    }
    if (changed) {
        Q_EMIT propertiesChanged();
    }
}

5. 代码可维护性改进

建议

// 添加更多文档注释
/**
 * @brief 设置颜色模式
 * @param v 颜色模式值,有效值为 "light", "dark", "auto"
 * @note 此操作会触发 colorModeChanged() 和 valueChanged() 信号
 * @warning 线程安全:此方法可以从任何线程调用
 */
void setColorMode(const QString &v);

// 使用枚举代替魔法字符串
enum class ColorMode {
    Light,
    Dark,
    Auto
};

QString colorModeToString(ColorMode mode);
ColorMode stringToColorMode(const QString& str);

6. 安全性改进

建议

// 添加输入验证
void setColorMode(const QString &v) {
    // 验证输入
    if (!isValidColorMode(v)) {
        qWarning() << "Invalid color mode:" << v;
        return;
    }
    // ... 其余代码
}

// 添加敏感操作保护
void resetToDefaults() {
    // 检查权限
    if (!hasResetPermission()) {
        qWarning() << "No permission to reset to defaults";
        return;
    }
    // ... 其余代码
}

7. 测试建议

建议添加以下测试用例:

  1. 多线程并发访问测试
  2. 快速创建/销毁对象测试
  3. 配置值边界条件测试
  4. 内存泄漏测试
  5. 信号发射频率测试

总结

代码整体质量良好,主要改进集中在线程安全和内存管理方面。建议优先实施线程安全改进和内存管理优化,这些改进可以显著提高代码的稳定性和性能。同时,添加更多的错误处理和输入验证可以提高代码的健壮性。

delete worker;
worker = nullptr;

auto data = safeData;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个赋值有啥必要吗

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

之前删的一块逻辑 等下删除了

if (!data->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid),
static_cast<int>(Data::Status::Initializing))) {
if (data->m_status.loadRelaxed() == static_cast<int>(Data::Status::Destroyed)) {
QMetaObject::invokeMethod(data, [data]() { delete data; }, Qt::QueuedConnection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data对象的生命管理是这样的:在状态变为 Initializing 之前,data被dconfig_org_deepin_dtk_preference对象负责销毁,在 Initializing 状态或 Succeeded 状态下,跟随config对象销毁。

所以这里直接返回就行,不用管 data 对象。

worker = nullptr;

auto data = safeData;
if (!data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要做这个判断,在判断 m_status 之后,可以对data对象做断言。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants