Skip to content

fix: fix system and app proxy is hide#503

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
ut003640:master
Feb 26, 2026
Merged

fix: fix system and app proxy is hide#503
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
ut003640:master

Conversation

@ut003640
Copy link
Contributor

@ut003640 ut003640 commented Feb 10, 2026

the system and app proxy must show on the control center

PMS: BUG-350619

Summary by Sourcery

Ensure system and app proxy controls remain visible and selectable in the network control center.

Bug Fixes:

  • Prevent system and app proxy items from being removed from the model so they stay visible in the UI.
  • Avoid duplicate or hidden proxy entries by filtering to show only the first occurrence of each item type and guarding model comparisons against null items.
  • Allow proxy-related switches in the system proxy page to always be interactive instead of conditionally disabled.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 10, 2026

Reviewer's Guide

Ensures system and app proxy controls are always visible in the control center by preventing their NetItem rows from being removed, filtering duplicate items in the proxy model, and making the System Proxy switches always enabled in QML.

Sequence diagram for NetItem removal with preserved proxy items

sequenceDiagram
    participant View
    participant NetItemModel
    participant ParentItem as NetItem_parent
    participant ChildItem as NetItem_child

    View->>NetItemModel: AboutToRemoveObject(parentItem, pos)
    NetItemModel->>ParentItem: getChild(pos)
    ParentItem-->>NetItemModel: childItem
    NetItemModel->>NetItemModel: needRemoveNodeItem(childItem)
    alt childItem is SystemProxyControlItem or AppProxyControlItem
        NetItemModel-->>View: skip beginRemoveRows
    else other item type
        NetItemModel->>View: beginRemoveRows
    end

    View->>NetItemModel: removeObject(child)
    NetItemModel->>NetItemModel: needRemoveNodeItem(child)
    alt child is SystemProxyControlItem or AppProxyControlItem
        NetItemModel-->>View: skip endRemoveRows
    else other item type
        NetItemModel->>View: endRemoveRows
    end
Loading

Class diagram for updated NetItemModel proxy filtering and row removal

classDiagram
    class NetItemModel {
        +rootChanged(root NetItem)
        +roleNames() QHash~int,QByteArray~
        +lessThan(source_left QModelIndex, source_right QModelIndex) bool
        +filterAcceptsRow(source_row int, source_parent QModelIndex) bool
        -needRemoveNodeItem(childItem NetItem) bool
        -AboutToRemoveObject(parentItem NetItem, pos int) void
        -removeObject(child NetItem) void
        -m_rowMap QMap~NetType_NetItemType,int~
    }

    class NetItem {
        +itemType() NetType_NetItemType
        +getChild(pos int) NetItem
    }

    class NetType_NetItemType {
        <<enumeration>>
        SystemProxyControlItem
        AppProxyControlItem
        otherTypes
    }

    NetItemModel --> NetItem : uses
    NetItemModel --> NetType_NetItemType : uses
    NetItem --> NetType_NetItemType : returns
Loading

File-Level Changes

Change Details Files
Prevent system and app proxy NetItem rows from being removed from the model
  • Introduce needRemoveNodeItem helper to detect system and app proxy items that must remain visible
  • Gate beginRemoveRows in AboutToRemoveObject based on needRemoveNodeItem for the child at the given position
  • Gate endRemoveRows in removeObject so removal notifications are skipped for protected items
dcc-network/operation/netitemmodel.cpp
Ensure unique display of NetItem rows and safer sorting in the proxy model
  • Override filterAcceptsRow to track first row per NetItemType via m_rowMap and filter out subsequent duplicates
  • Add a mutable QMap member m_rowMap to store the first row index per NetItemType
  • Add null checks for left and right items in lessThan before comparing itemType values
  • Move netitem.h include from cpp into header so NetItemType and NetItem are available to the proxy model
dcc-network/operation/netitemmodel.cpp
dcc-network/operation/netitemmodel.h
Make System Proxy toggles always user-interactable in the QML UI
  • Force the main system proxy enable switch to be always enabled instead of relying on netItem.enabledable
  • Force the method selection switch to be always enabled instead of relying on netItem.enabledable
dcc-network/qml/PageSystemProxy.qml

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

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 found 1 issue, and left some high level feedback:

  • The filterAcceptsRow implementation keeps state in m_rowMap across calls but never resets it, which can lead to incorrect filtering as the model changes over time; consider clearing or rebuilding this map when the source model changes or on each filter pass instead of relying on mutable state.
  • Using NetType::NetItemType as the key in m_rowMap ignores the parent index, so only one row per type will ever be shown even if logically distinct items exist under different parents; if that’s not intended, include the parent (or full index path) in the key.
  • In filterAcceptsRow, you don't call the base QSortFilterProxyModel::filterAcceptsRow, so any existing filtering behavior is bypassed; if additional conditions should be combined with the default logic, explicitly delegate to the base implementation.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `filterAcceptsRow` implementation keeps state in `m_rowMap` across calls but never resets it, which can lead to incorrect filtering as the model changes over time; consider clearing or rebuilding this map when the source model changes or on each filter pass instead of relying on mutable state.
- Using `NetType::NetItemType` as the key in `m_rowMap` ignores the parent index, so only one row per type will ever be shown even if logically distinct items exist under different parents; if that’s not intended, include the parent (or full index path) in the key.
- In `filterAcceptsRow`, you don't call the base `QSortFilterProxyModel::filterAcceptsRow`, so any existing filtering behavior is bypassed; if additional conditions should be combined with the default logic, explicitly delegate to the base implementation.

## Individual Comments

### Comment 1
<location> `dcc-network/operation/netitemmodel.cpp:233-238` </location>
<code_context>
+
 bool NetItemModel::lessThan(const QModelIndex &source_left, const QModelIndex &source_right) const
 {
     NetItem *leftItem = static_cast<NetItem *>(source_left.internalPointer());
+    if (!leftItem)
+        return false;
+
     NetItem *rightItem = static_cast<NetItem *>(source_right.internalPointer());
+    if (!rightItem)
+        return false;
+
</code_context>

<issue_to_address>
**issue (bug_risk):** Handling null pointers in `lessThan` by returning `false` can violate strict weak ordering.

Returning `false` when either pointer is null means that for a null vs non-null pair, both `lessThan(left, right)` and `lessThan(right, left)` can be `false`, which violates strict weak ordering required by `QSortFilterProxyModel` and can cause unstable sorting. Consider defining a consistent ordering for nulls (e.g., always less or always greater than non-null) so comparisons remain deterministic.
</issue_to_address>

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.

Comment on lines 233 to 238
NetItem *leftItem = static_cast<NetItem *>(source_left.internalPointer());
if (!leftItem)
return false;

NetItem *rightItem = static_cast<NetItem *>(source_right.internalPointer());
if (!rightItem)
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Handling null pointers in lessThan by returning false can violate strict weak ordering.

Returning false when either pointer is null means that for a null vs non-null pair, both lessThan(left, right) and lessThan(right, left) can be false, which violates strict weak ordering required by QSortFilterProxyModel and can cause unstable sorting. Consider defining a consistent ordering for nulls (e.g., always less or always greater than non-null) so comparisons remain deterministic.

Copy link
Contributor

@caixr23 caixr23 left a comment

Choose a reason for hiding this comment

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

数据重复改数据,不要改NetItemModel

@ut003640 ut003640 force-pushed the master branch 3 times, most recently from 6a31233 to c4aa8b1 Compare February 26, 2026 03:30
caixr23
caixr23 previously approved these changes Feb 26, 2026
the system and app proxy must show on the control center

PMS: BUG-350619
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的 git diff 展示了对网络类型定义 (nettype.h) 和网络管理器线程私有实现 (netmanagerthreadprivate.cpp) 的修改。主要目的是引入一个新的标志位 Net_SysProxyAlwaysShow,用于控制系统代理项是否总是显示,而不是根据系统代理是否存在来动态显示。

以下是对这段代码的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • 位掩码值的合理性

    • 观察Net_SysProxyAlwaysShow = 0x08000000
    • 分析:前一个标志位 Net_VPNChildren0x04000000 (即第 26 位)。新标志位 0x08000000 是第 27 位。这是一个合法的按 2 的幂次方增长的位掩码值,没有与其他位重叠。
    • 建议:逻辑正确。
  • 条件判断逻辑

    • 观察:在 cpp 文件中,使用了 if (!m_flags.testFlags(NetType::NetManagerFlag::Net_SysProxyAlwaysShow)) 来包裹原有的逻辑。
    • 分析testFlags 通常用于检查是否包含特定标志。这里的逻辑是:如果标志位中不包含 Net_SysProxyAlwaysShow(即未开启“总是显示”模式),则执行原有的逻辑(根据 systemProxyExist 来决定显示/隐藏及连接信号)。
    • 建议:逻辑通顺。这意味着如果 Net_DccFlags 等组合标志中加入了 Net_SysProxyAlwaysShow,系统代理项将不再响应 systemProxyExistChanged 信号,从而保持显示状态(假设默认是显示的,或者由其他逻辑控制初始状态)。这符合“总是显示”的语义。

2. 代码质量

  • 版权年份更新

    • 观察:将 SPDX-FileCopyrightText2019 - 2022 更新为 2019 - 2026
    • 建议:通常建议更新到当前年份(例如 2023 或 2024)或者保留 2022 除非有特定理由预测到 2026 年。直接写到 2026 年略显随意,建议改为当前实际年份。
  • 代码可读性与维护性

    • 观察Net_DccFlags 的定义行变得很长。
    • 建议:虽然 C++ 允许很长的行,但为了提高可读性,可以考虑将 Net_DccFlags 的定义拆分为多行,或者按照功能分组(如设备类、代理类、VPN类),这样后续添加或删除标志时更容易进行对比。
  • 信号连接的上下文

    • 观察:在 if 块外部连接了 proxyMethodChanged, autoProxyChanged, proxyChanged,但在内部连接了 systemProxyExistChanged
    • 建议:这种不一致性是有意的(因为前者无论是否总是显示都需要监听代理内容变化),但建议添加注释说明为什么 systemProxyExistChanged 的连接被特殊处理,以防止未来的维护者误将其移出 if 块。

3. 代码性能

  • 运行时开销
    • 分析testFlags 是一个极快的位运算操作。connect 操作只在初始化时执行一次。
    • 建议:性能影响可以忽略不计。

4. 代码安全

  • 潜在的内存/资源泄漏风险
    • 分析:目前的修改逻辑是:如果不“总是显示”,就连接 systemProxyExistChanged 信号。反之,则不连接。
    • 潜在问题:如果 m_flags 在运行时发生变化(虽然 flags 通常是初始化时确定的,但如果不保证不可变),或者 NetManagerThreadPrivate 对象复用逻辑中 doInit 被多次调用,可能会出现重复连接信号的情况,或者之前的连接没有被断开。
    • 建议
      1. 确保 doInit 不会被多次调用,或者在调用前确保旧的连接已断开(使用 disconnectQObject::disconnect)。
      2. 如果 m_flags 可能动态变化,需要添加逻辑来动态连接/断开信号。但从代码上下文看,flags 似乎是构造时确定的,因此风险较低。
      3. Qt 5 的连接语法:使用 connect(sender, &Sender::signal, this, &Receiver::slot) 这种函数指针语法在信号重载时可能会有二义性,但在 Qt 5.7+ 中对于没有重载的信号是安全且推荐的。这里看起来没有问题。

总结与改进建议

总体来说,这段代码实现了预期的功能,逻辑正确。以下是具体的改进建议:

  1. 修正版权年份:将 2026 改为当前年份(如 20232024)。
  2. 添加注释:在 if (!m_flags.testFlags(...)) 块之前添加注释,解释为什么这里需要条件连接信号。
    // 如果配置未指定"总是显示系统代理",则需要根据系统代理是否存在来动态控制显示/隐藏
    if (!m_flags.testFlags(NetType::NetManagerFlag::Net_SysProxyAlwaysShow)) {
        // ...
    }
  3. 格式化长行:优化 nettype.hNet_DccFlags 的格式,使其更易读。
  4. 检查 doInit 调用:确认 doInit 的调用时机,确保不会导致信号重复连接。如果存在多次调用的风险,建议在连接前先使用 disconnect(...) 断开旧连接。

@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 Feb 26, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 241ba60 into linuxdeepin:master Feb 26, 2026
16 of 18 checks passed
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