Skip to content

Conversation

@ut003640
Copy link
Contributor

@ut003640 ut003640 commented Jan 28, 2026

删除VPN的时候,会先删除节点,删除节点会先断开信号槽函数的连接,然后才发送VPN状态变化的信号,由于此时信号已经断开,发送的信号无法收到,所以状态没有变化

Log: 修复删除VPN后任务栏图标没有变化的问题
Influence: 从控制中心删除已经连接的VPN,观察任务栏图标中是否还存在VPN图标
Bug: https://pms.uniontech.com/bug-view-341927.html

Summary by Sourcery

Bug Fixes:

  • Ensure taskbar VPN icon updates correctly when a connected VPN is deleted from the control center by reconnecting VPN status change signals between NetManager internals and the status UI.

删除VPN的时候,会先删除节点,删除节点会先断开信号槽函数的连接,然后才发送VPN状态变化的信号,由于此时信号已经断开,发送的信号无法收到,所以状态没有变化

Log: 修复删除VPN后任务栏图标没有变化的问题
Influence: 从控制中心删除已经连接的VPN,观察任务栏图标中是否还存在VPN图标
Bug: https://pms.uniontech.com/bug-view-341927.html
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 28, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Reconnects and propagates VPN status change signals from NetManagerPrivate through NetManager to NetStatus so that the taskbar icon updates correctly when a connected VPN is deleted.

Sequence diagram for VPN deletion status propagation

sequenceDiagram
    actor ControlCenterUser
    participant ControlCenter
    participant NetManager as NetManager
    participant NetManagerPrivate as NetManagerPrivate
    participant NetStatus as NetStatus
    participant TaskbarIcon as TaskbarIcon

    ControlCenterUser ->> ControlCenter: deleteVPN(vpnId)
    ControlCenter ->> NetManagerPrivate: removeVpnConnection(vpnId)
    NetManagerPrivate ->> NetManagerPrivate: disconnectSignalsForVpn(vpnId)
    NetManagerPrivate ->> NetManagerPrivate: performVpnRemoval(vpnId)

    NetManagerPrivate -->> NetManager: vpnStatusChanged()
    NetManager -->> NetStatus: vpnStatusChanged()
    NetStatus ->> NetStatus: updateVpnAndProxyStatus()
    NetStatus -->> TaskbarIcon: setVpnStatus(disconnected)
Loading

Class diagram for updated VPN status change signal chain

classDiagram
    class NetManagerPrivate {
        +request()
        +vpnStatusChanged()
        +init(flags)
        +removeVpnConnection(vpnId)
    }

    class NetManager {
        -QScopedPointer_NetManagerPrivate d_ptrNetManager
        +NetManager(flags, parent)
        +request()
        +vpnStatusChanged()
        +toControlCenter()
        +netCheckAvailableChanged(netCheckAvailable)
        +primaryConnectionTypeChanged(type)
    }

    class NetStatus {
        +onChildAdded(child)
        +updateVpnAndProxyStatus()
    }

    class NetDeviceItem {
        +nameChanged()
        +enabledableChanged()
        +enabledChanged()
    }

    NetManagerPrivate <|-- NetManager : has_pointer_to
    NetManagerPrivate --> NetManager : emits_vpnStatusChanged
    NetManager --> NetStatus : emits_vpnStatusChanged
    NetStatus --> NetDeviceItem : connects_signals
    NetDeviceItem --> NetStatus : nameChanged
    NetDeviceItem --> NetStatus : enabledableChanged
    NetDeviceItem --> NetStatus : enabledChanged
    NetStatus --> NetStatus : updateVpnAndProxyStatus_calls
Loading

File-Level Changes

Change Details Files
Re‑enable and wire VPN status change signal propagation from NetManagerPrivate to NetStatus to trigger taskbar icon updates.
  • Declare vpnStatusChanged as an active Qt slot on NetManager instead of commenting it out.
  • Connect NetManagerPrivate::vpnStatusChanged to NetManager::vpnStatusChanged using a queued connection in the NetManager constructor.
  • Reconnect NetManager::vpnStatusChanged to NetStatus::updateVpnAndProxyStatus when a VPN device item is added so VPN status UI refreshes on status changes, including deletion.
net-view/operation/netmanager.h
net-view/operation/netmanager.cpp
net-view/window/netstatus.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码主要涉及在 Qt 框架中启用 VPN 状态变更信号的连接。以下是对这段 git diff 的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • 符合 Qt 信号槽机制
    • netmanager.cpp 中,连接了 NetManagerPrivatevpnStatusChanged 信号到 NetManager 的同名信号,使用了 Qt::QueuedConnection。这是跨线程通信的标准做法,逻辑正确。
    • netmanager.h 中,将 vpnStatusChanged 从注释状态恢复为声明,使其成为一个有效的公共槽函数或信号(视具体实现而定,此处作为信号转发的槽或信号本身均可)。
    • netstatus.cpp 中,连接了 NetManagervpnStatusChanged 信号到 NetStatusupdateVpnAndProxyStatus 槽函数。逻辑链条完整:底层变化 -> 信号转发 -> UI 更新。

2. 代码质量

  • 信号命名一致性:代码中保持了 vpnStatusChanged 命名的一致性,这符合 Qt 的命名规范,提高了代码的可读性。
  • 代码清理:取消注释被屏蔽的代码是正确的,因为功能需要被启用。这表明代码正在被维护和更新。
  • 建议:在 netmanager.h 中,vpnStatusChanged() 被声明在 public Q_SLOTS 下。
    • 如果 NetManager 只是为了转发 NetManagerPrivate 的信号(即 NetManager 自身不发射该信号,只是作为接收者再发射),那么将其放在 signals 部分可能更符合语义。
    • 如果它作为一个槽函数供外部调用以触发状态更新,则当前位置无误。根据 netmanager.cpp 中的 connect(..., this, &NetManager::vpnStatusChanged, ...) 写法,这里看起来是将 NetManager::vpnStatusChanged 作为(或者说是信号的转发者),因此放在 Q_SLOTS 在逻辑上是通的,但在 Qt 中通常这种纯转发逻辑往往直接定义在 signals 区,并使用 emit 关键字。如果当前实现是直接通过函数指针调用实现转发,放在 slots 也是可以的,但需注意不要混淆信号和槽的概念。

3. 代码性能

  • 连接类型:使用了 Qt::QueuedConnection
    • 优点:这确保了槽函数在接收者的线程中执行,避免了多线程竞争条件,提高了稳定性。
    • 注意:如果 NetManagerNetManagerPrivate 处于同一线程,Qt::AutoConnection(默认值)通常会更高效(直接调用),因为它会省去排队的事件循环开销。不过,如果设计上明确涉及跨线程(d_ptr 模式常暗示这一点),或者为了确保线程安全的一致性,显式使用 Qt::QueuedConnection 是合理的。

4. 代码安全

  • 空指针检查:在 netmanager.cpp 构造函数中,connect(d_ptrNetManager.get(), ...) 使用了 d_ptrNetManager.get()
    • 由于这是在构造函数中,且 d_ptrNetManager 是成员变量,此时它应该已经被初始化(通常在初始化列表中)。如果 d_ptrNetManager 初始化失败(例如内存不足),程序在到达这一行之前可能已经抛出异常。因此这里的空指针风险较低,但需确保 d_ptrNetManager 的初始化顺序正确。
  • 重复连接风险
    • netstatus.cpponChildAdded 函数中进行连接。
    • 潜在问题:如果 onChildAdded 被多次调用,或者同一个 NetManager 实例被重复添加/处理,可能会导致 vpnStatusChanged 信号被多次连接到 updateVpnAndProxyStatus。当信号发射时,槽函数会被执行多次,这可能导致性能浪费或逻辑错误(如多次刷新 UI)。
    • 改进建议:建议在连接前使用 disconnect 断开旧连接,或者使用 QObject::blockSignals 临时阻塞,或者确保逻辑上不会重复进入此代码块。例如:
      // 断开可能存在的旧连接,防止重复连接
      disconnect(m_manager, &NetManager::vpnStatusChanged, this, &NetStatus::updateVpnAndProxyStatus);
      // 建立新连接
      connect(m_manager, &NetManager::vpnStatusChanged, this, &NetStatus::updateVpnAndProxyStatus);

总结

这段代码的主要目的是打通 VPN 状态变更的通知链路。整体实现符合 Qt 规范,逻辑正确。主要的改进点在于关注 netstatus.cpp 中可能存在的信号重复连接问题,以及在 netmanager.h 中确认 vpnStatusChanged 作为信号还是槽的语义准确性。

Copy link

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

Hey - I've left some high level feedback:

  • In NetStatus::onChildAdded, the connection from m_manager's vpnStatusChanged to updateVpnAndProxyStatus will be created every time a VpnControlItem is added, which can lead to the slot being invoked multiple times for a single signal; consider using Qt::UniqueConnection or moving this connection to a place where it is made only once.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `NetStatus::onChildAdded`, the connection from `m_manager`'s `vpnStatusChanged` to `updateVpnAndProxyStatus` will be created every time a `VpnControlItem` is added, which can lead to the slot being invoked multiple times for a single signal; consider using `Qt::UniqueConnection` or moving this connection to a place where it is made only once.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caixr23, ut003640

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

@ut003640
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 28, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit a2b3532 into linuxdeepin:master Jan 28, 2026
16 of 18 checks passed
@ut003640 ut003640 deleted the bug-341927-new branch January 29, 2026 08:51
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.

3 participants