fix: fix system and app proxy is hide#503
Conversation
Reviewer's GuideEnsures 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 itemssequenceDiagram
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
Class diagram for updated NetItemModel proxy filtering and row removalclassDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
filterAcceptsRowimplementation keeps state inm_rowMapacross 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::NetItemTypeas the key inm_rowMapignores 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 baseQSortFilterProxyModel::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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| NetItem *leftItem = static_cast<NetItem *>(source_left.internalPointer()); | ||
| if (!leftItem) | ||
| return false; | ||
|
|
||
| NetItem *rightItem = static_cast<NetItem *>(source_right.internalPointer()); | ||
| if (!rightItem) |
There was a problem hiding this comment.
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.
6a31233 to
c4aa8b1
Compare
the system and app proxy must show on the control center PMS: BUG-350619
deepin pr auto review这段代码的 以下是对这段代码的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面: 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议总体来说,这段代码实现了预期的功能,逻辑正确。以下是具体的改进建议:
|
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
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: