Skip to content

fix: fixed the wired device show error#514

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

fix: fixed the wired device show error#514
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
ut003640:master

Conversation

@ut003640
Copy link
Contributor

@ut003640 ut003640 commented Mar 4, 2026

because we used the macro, so the condition is error, the wired device which is off will show

PMS: BUG-308755

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.

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

Please try again later or upgrade to continue using Sourcery

because we used the macro, so the condition is error, the wired device which is off will show

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

deepin pr auto review

这段帕克-2.0(Spark 2.0)对 git diff 输入或代码函数进行仔细的审查,并对语法逻辑,代码质量,代码性能和代码安全提出改进意见。请用中文回答。

代码审查报告

总体评价

本次代码变更主要涉及移除 ENABLE_DEEPIN_NMQT 条件编译选项及其相关代码,统一了设备管理的逻辑。整体来看,代码变更简化了条件编译,提高了代码的可维护性,但还有一些可以改进的地方。

详细审查

1. CMakeLists.txt 变更

变更内容

  • 移除了 ENABLE_DEEPIN_NMQT 选项定义
  • 移除了相关的条件编译代码

评价

  • 语法逻辑:正确,移除了不再需要的条件编译选项
  • 代码质量:提高了代码一致性,减少了条件分支
  • 建议:移除选项后,应确保 DEEPIN_NMQT_TEST 变量仍然有定义和使用,否则也应移除相关代码

2. networkmanagerprocesser.cpp 变更

2.1 版权信息更新

变更内容

-// SPDX-FileCopyrightText: 2018 - 2022 UnionTech Software Technology Co., Ltd.
+// SPDX-FileCopyrightText: 2018 - 2026 UnionTech Software Technology Co., Ltd.

评价

  • 代码质量:版权年份更新合理,但建议使用当前年份或更合理的范围(如2018-2024)

2.2 createOrRemoveDevice 函数变更

变更内容

-    if (device->managed()
-#ifdef USE_DEEPIN_NMQT
-                && ((device->interfaceFlags() & DEVICE_INTERFACE_FLAG_UP) ||
-                device->type() == NetworkManager::Device::Wifi)
-#endif
-            ) {
+    if (device->managed() && ((device->interfaceFlags() & DEVICE_INTERFACE_FLAG_UP) || device->type() == NetworkManager::Device::Wifi)) {

评价

  • 语法逻辑:正确,移除了条件编译,统一了设备管理逻辑
  • 代码质量:简化了代码结构,提高了可读性
  • 潜在问题:需要确保这个变更在所有平台上都能正常工作,特别是之前使用 USE_DEEPIN_NMQT 的平台

2.3 onDeviceAdded 函数变更

变更内容

-#ifdef USE_DEEPIN_NMQT
     connect(currentDevice.get(), &NetworkManager::Device::interfaceFlagsChanged, currentDevice.get(), [ uni, this ] {
         createOrRemoveDevice(uni);
     });
-#endif

评价

  • 语法逻辑:正确,移除了条件编译
  • 代码质量:统一了事件处理逻辑
  • 潜在问题:需要确保在所有平台上 interfaceFlagsChanged 信号都能正常工作

改进建议

  1. 代码一致性

    • 考虑将类似的条件编译检查全部移除,确保代码一致性
    • 检查项目中是否还有其他使用 USE_DEEPIN_NMQT 的地方
  2. 代码可读性

    • 考虑将复杂的条件判断提取为单独的函数,提高可读性:
      bool shouldManageDevice(NetworkManager::Device::Ptr device) {
          return device->managed() && 
                 ((device->interfaceFlags() & DEVICE_INTERFACE_FLAG_UP) || 
                  device->type() == NetworkManager::Device::Wifi);
      }
  3. 代码性能

    • 当前代码没有明显的性能问题
    • 如果频繁调用 createOrRemoveDevice,可以考虑添加缓存机制
  4. 代码安全

    • 确保 device 指针在使用前已经检查有效性
    • 考虑添加空指针检查:
      if (!device) {
          qWarning() << "Invalid device pointer";
          return;
      }
  5. 测试建议

    • 添加单元测试覆盖新的设备管理逻辑
    • 在不同平台上测试设备添加/移除功能
  6. 文档更新

    • 更新相关文档,说明设备管理逻辑的变化
    • 添加注释说明为什么统一使用 interfaceFlagsChanged 信号

总结

这次代码变更简化了条件编译,提高了代码的一致性和可维护性。主要风险在于需要确保新的统一逻辑在所有平台上都能正常工作。建议进行充分的测试,特别是在不同平台和不同设备类型上的测试。

@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

ut003640 commented Mar 5, 2026

/force

@ut003640
Copy link
Contributor Author

ut003640 commented Mar 5, 2026

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Mar 5, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 80d039c into linuxdeepin:master Mar 5, 2026
16 of 19 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