Skip to content

feat(dccquickdbusinterface): add enabled property to control D-Bus connection#3256

Open
18202781743 wants to merge 1 commit into
masterfrom
worktree-dccquickdbusinterface-enabled
Open

feat(dccquickdbusinterface): add enabled property to control D-Bus connection#3256
18202781743 wants to merge 1 commit into
masterfrom
worktree-dccquickdbusinterface-enabled

Conversation

@18202781743
Copy link
Copy Markdown
Contributor

Summary

  • Add enabled Q_PROPERTY to DccQuickDBusInterface for dynamic D-Bus connection control
  • Implement connectToDBus() and disconnectFromDBus() private methods
  • Track signal callbacks for proper disconnection
  • Add m_completed flag to handle componentComplete() ordering

Changes

  • Add enabled property with getter, setter, and notification signal
  • When enabled=false, disconnects from D-Bus signals and properties
  • When enabled=true, reconnects to D-Bus

Test plan

  • Verify D-Bus connections are established when enabled=true (default)
  • Verify D-Bus connections are disconnected when enabled=false
  • Verify reconnection works when toggling enabled

🤖 Generated with Claude Code

…nnection

Add enabled property to DccQuickDBusInterface to allow dynamic control
of D-Bus connections. When enabled is false, the interface disconnects
from D-Bus signals and properties. When enabled is true, it reconnects.

Changes:
- Add enabled Q_PROPERTY with getter, setter, and notification signal
- Add connectToDBus() and disconnectFromDBus() private methods
- Track signal callbacks for proper disconnection
- Add m_completed flag to handle componentComplete() ordering
- Add enabled to ReservedPropertyNames list
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Sorry @18202781743, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743

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
Copy link
Copy Markdown

deepin pr auto review

这份代码为 DccQuickDBusInterface 增加了 enabled 属性,以支持动态连接和断开 DBus 信号,整体逻辑清晰,结构合理。但在代码的安全性、内存管理、逻辑严谨性以及 Qt 惯用写法上,还有一些可以改进和优化的空间。

以下是详细的审查意见和改进建议:

1. 代码安全与内存管理(严重问题)

问题:disconnectFromDBus 中存在内存泄漏和悬空指针风险
connectToDBus 中,代码通过 new 创建了 DccDBusSignalCallback 对象,但在 disconnectFromDBus 中,仅仅调用了 p_ptr->m_signalCallbacks.clear()没有释放这些对象的内存,这会导致内存泄漏。
更严重的是,如果未来有其他逻辑单独持有或引用了这些 callback,由于它们没有被 delete,可能会引发未定义行为。

改进建议:
在清理列表前,需要手动释放这些 callback 的内存。由于 Qt 的信号槽机制在断开连接后不再调用 slot,所以直接 delete 是安全的。

void DccQuickDBusInterface::disconnectFromDBus()
{
    // 释放内存
    for (const auto &[callback, signalName] : std::as_const(p_ptr->m_signalCallbacks)) {
        delete callback; 
    }
    p_ptr->m_signalCallbacks.clear();
    // ... 其他逻辑
}

2. 语法与代码规范(建议优化)

问题:魔法数字 2
connectToDBus 中,if (m.methodType() == 2 ...) 使用了魔法数字。虽然 2 在 Qt 中代表 QMetaMethod::Signal,但直接使用数字会降低代码可读性。

改进建议:
使用枚举值替换。

// 修改前
if (m.methodType() == 2 && m.name().startsWith("on")) {

// 修改后
if (m.methodType() == QMetaMethod::Signal && m.name().startsWith("on")) {

3. 逻辑严谨性(重要问题)

问题:enabled 默认值为 true,但初始状态并未连接 DBus
dccquickdbusinterface_p.h 中,m_enabled 默认初始化为 true。但是,在 componentComplete 之前的生命周期中,DBus 实际上是未连接的。
如果用户在 QML 中这样写:DccQuickDBusInterface { enabled: true },QML 引擎在属性赋值时会调用 setEnabled(true)。由于 p_ptr->m_enabled 已经是 truesetEnabled 会因为 if (p_ptr->m_enabled == enabled) return; 这行代码而直接返回,导致 connectToDBus 永远不会被调用。

改进建议:
m_enabled 的默认值设为 false。这样当 QML 声明 enabled: true 时,才会真正触发 setEnabled(true) 并执行连接逻辑。

// dccquickdbusinterface_p.h
bool m_enabled = false; // 默认改为 false

4. 代码性能与健壮性(建议优化)

问题:disconnectFromDBus 中断开属性变更信号的硬编码
connectToDBus 中,PropertiesInterfacePropertiesChanged 使用了变量(推测是 static const),但在 disconnectFromDBus 中却直接使用了未声明的标识符(假设是宏或全局变量)。这种不对称的写法容易出错,且如果参数不匹配,QDBusConnection::disconnect 会返回 false 导致断开失败。

改进建议:
确保 disconnect 时的参数与 connect 时完全一致,建议提取为常量或确保使用相同的变量名。

5. 代码安全与防御性编程

问题:缺少对 connectToDBusdisconnectFromDBus 调用状态的校验
如果由于某种 Bug 导致 connectToDBus 被连续调用两次,会导致信号被连接两次,回调也会被执行两次。同理,disconnectFromDBus 重复调用也可能引发异常。

改进建议:
在函数入口处增加状态检查。

void DccQuickDBusInterface::connectToDBus()
{
    if (p_ptr->m_signalCallbacks.isEmpty() && !p_ptr->m_propertyConnected) {
        // 可以连接
    } else {
        return; // 已经连接,避免重复连接
    }
    // ...
}

void DccQuickDBusInterface::disconnectFromDBus()
{
    if (p_ptr->m_signalCallbacks.isEmpty() && !p_ptr->m_propertyConnected) {
        return; // 已经断开,无需操作
    }
    // ...
}

综合修改后的代码参考

dccquickdbusinterface_p.h

// ... 其他代码
    QList<QPair<DccDBusSignalCallback *, QString>> m_signalCallbacks;
    bool m_propertyConnected = false;
    bool m_completed = false;
    bool m_enabled = false; // 修改:默认值改为 false,确保 QML 赋值时能触发逻辑
// ... 其他代码

dccquickdbusinterface.cpp

// ... setEnabled 逻辑保持不变 ...

void DccQuickDBusInterface::connectToDBus()
{
    // 防御性编程:避免重复连接
    if (!p_ptr->m_signalCallbacks.isEmpty() || p_ptr->m_propertyConnected)
        return;

    const QMetaObject *mo = this->metaObject();
    const int mcount = mo->methodCount();
    for (int i = mo->methodOffset(); i < mcount; ++i) {
        const QMetaMethod &m = mo->method(i);
        // 修改:消除魔法数字
        if (m.methodType() == QMetaMethod::Signal && m.name().startsWith("on")) {
            const QString signalName = m.name().mid(2);
            DccDBusSignalCallback *callback = new DccDBusSignalCallback(m, this);
            p_ptr->m_connection.connect(p_ptr->m_service, p_ptr->m_path, p_ptr->m_interface, signalName, callback, SLOT(returnMethod(QDBusMessage)));
            p_ptr->m_signalCallbacks.append({ callback, signalName });
        }
    }
    if (!p_ptr->m_mapProperties.isEmpty()) {
        p_ptr->m_connection.connect(p_ptr->m_service,
                                    p_ptr->m_path,
                                    PropertiesInterface,
                                    PropertiesChanged,
                                    p_ptr,
                                    SLOT(onPropertiesChanged(QString, QVariantMap, QStringList)));
        p_ptr->m_propertyConnected = true;
    }
}

void DccQuickDBusInterface::disconnectFromDBus()
{
    // 防御性编程:无需断开时直接返回
    if (p_ptr->m_signalCallbacks.isEmpty() && !p_ptr->m_propertyConnected)
        return;

    // 修改:释放回调对象的内存,防止内存泄漏
    for (const auto &[callback, signalName] : std::as_const(p_ptr->m_signalCallbacks)) {
        p_ptr->m_connection.disconnect(p_ptr->m_service,
                                       p_ptr->m_path,
                                       p_ptr->m_interface,
                                       signalName,
                                       callback,
                                       SLOT(returnMethod(QDBusMessage)));
        delete callback; 
    }
    p_ptr->m_signalCallbacks.clear();

    if (p_ptr->m_propertyConnected) {
        p_ptr->m_connection.disconnect(p_ptr->m_service,
                                       p_ptr->m_path,
                                       PropertiesInterface,
                                       PropertiesChanged,
                                       p_ptr,
                                       SLOT(onPropertiesChanged(QString, QVariantMap, QStringList)));
        p_ptr->m_propertyConnected = false;
    }
}

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.

2 participants