Skip to content

Conversation

@GongHeng2017
Copy link
Contributor

@GongHeng2017 GongHeng2017 commented Oct 10, 2025

-- The network control not have logic name and address,
so disable failed.

Log: fix issue
Bug: https://pms.uniontech.com/bug-view-334701.html

Summary by Sourcery

Add logic to conditionally disable network interfaces based on availability and update the UI to hide the disable action when unsupported

New Features:

  • Introduce canDisable() in DeviceNetwork to determine if a network device can be disabled

Bug Fixes:

  • Prevent failures when disabling network devices without a valid system path

Enhancements:

  • TableWidget now checks the canDisable flag to remove the disable option for unsupported devices
  • PageMultiInfo includes the canDisable state in menu control data to drive UI behavior

@sourcery-ai
Copy link

sourcery-ai bot commented Oct 10, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds a canDisable() check on DeviceNetwork, exposes it through the UI model, and filters the disable option in the context menu accordingly.

Sequence diagram for context menu disable option filtering

sequenceDiagram
participant "User"
participant "TableWidget"
participant "QTableWidgetItem"
participant "DeviceNetwork"

"User"->>"TableWidget": Right-clicks on network device row
"TableWidget"->>"QTableWidgetItem": Get data (canDisable)
"TableWidget"->>"DeviceNetwork": canDisable()
"DeviceNetwork"-->>"TableWidget": true/false
"TableWidget"->>"TableWidget": If canDisable == false, remove disable action from menu
"TableWidget"->>"User": Show context menu
Loading

Class diagram for DeviceNetwork changes

classDiagram
class DeviceNetwork {
    +QString hwAddress()
    +bool canDisable()
}
DeviceNetwork --|> DeviceBaseInfo
Loading

File-Level Changes

Change Details Files
Introduce canDisable() method to validate network disable eligibility
  • Add canDisable() prototype to DeviceNetwork header
  • Implement canDisable() in cpp to return false if m_SysPath is empty
deepin-devicemanager/src/DeviceManager/DeviceNetwork.h
deepin-devicemanager/src/DeviceManager/DeviceNetwork.cpp
Propagate disable flag to UI model
  • Append network.canDisable() value as string to menuControl in PageMultiInfo
deepin-devicemanager/src/Page/PageMultiInfo.cpp
Conditionally hide disable action in context menu based on flag
  • Retrieve canDisableForNetwork from item data
  • Remove disable action when flag is false before menu exec
deepin-devicemanager/src/Widget/TableWidget.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

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 there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `deepin-devicemanager/src/DeviceManager/DeviceNetwork.cpp:251-257` </location>
<code_context>
     return m_MACAddress;
 }

+bool DeviceNetwork::canDisable()
+{
+    if (m_SysPath.isEmpty()) {
+        return false;
+    }
+    return true;
+}
+
</code_context>

<issue_to_address>
**suggestion:** canDisable() could be simplified.

Consider replacing the method body with 'return !m_SysPath.isEmpty();' for improved clarity and brevity.

```suggestion
bool DeviceNetwork::canDisable()
{
    return !m_SysPath.isEmpty();
}
```
</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.

-- The network control not have logic name and address,
   so disable failed.

Log: fix issue
Bug: https://pms.uniontech.com/bug-view-334701.html
@GongHeng2017 GongHeng2017 force-pushed the 202510100957-eagle-fix branch from 2aeed58 to 2e646ac Compare October 10, 2025 03:20
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的审查意见如下:

  1. 代码逻辑和功能:
  • 新增了canDisable()函数用于判断网络设备是否可以禁用,这是一个合理的功能增强
  • PageMultiInfo.cpp中将canDisable()的结果转换为字符串添加到菜单控制列表中
  • PageSingleInfo.cppTableWidget.cpp中根据canDisable()的值来决定是否显示"启用"菜单项
  1. 代码质量改进建议:
  • canDisable()函数的实现过于简单,可以考虑添加更详细的检查逻辑,比如检查设备状态、权限等
  • PageMultiInfo.cpp中将布尔值转换为字符串"true"/"false"不够优雅,可以考虑直接使用布尔值
  • TableWidget.cpp中使用Qt::UserRole + 3作为索引来存储canDisable值,建议使用枚举或常量来定义这些角色索引,提高代码可读性
  1. 代码性能:
  • 目前实现没有明显的性能问题,因为canDisable()只是简单检查一个成员变量
  • 如果未来需要添加更复杂的检查逻辑,建议考虑缓存结果,避免重复计算
  1. 代码安全:
  • 需要确保m_SysPath的检查是全面的,不仅仅是检查是否为空
  • 在移除菜单项前,应该确保mp_Enable指针有效,避免空指针解引用
  1. 具体改进建议:
// 在DeviceNetwork.h中定义常量
static const int CanDisableRole = Qt::UserRole + 3;

// 改进canDisable()的实现
bool DeviceNetwork::canDisable()
{
    // 检查系统路径是否有效
    if (m_SysPath.isEmpty()) {
        return false;
    }
    
    // 可以添加更多检查,比如设备状态、权限等
    // 例如:检查设备是否处于活动状态
    if (!m_IsActive) {
        return false;
    }
    
    return true;
}

// 在PageMultiInfo.cpp中使用布尔值而非字符串
menuControl.append(network->canDisable());

// 在TableWidget.cpp中使用常量
QVariant canDisableForNetwork = item->data(TableWidget::CanDisableRole);
if (canDisableForNetwork.isValid() && !canDisableForNetwork.toBool()) {
    mp_Menu->removeAction(mp_Enable);
}
  1. 其他建议:
  • 考虑添加注释说明canDisable()函数的具体判断条件
  • 在菜单操作前添加日志记录,便于调试和问题追踪
  • 考虑将菜单项的显示/隐藏逻辑封装成单独的函数,提高代码复用性

总体而言,这个功能实现是合理的,但可以通过上述建议进一步提高代码质量、可读性和健壮性。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, GongHeng2017, max-lvs

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

@GongHeng2017
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Oct 10, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit f720214 into linuxdeepin:develop/eagle Oct 10, 2025
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.

4 participants