-
Notifications
You must be signed in to change notification settings - Fork 50
feat: refactor network device status display and CMake version #486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideRefactors network device status presentation into a reusable QML component, centralizes wireless band configuration handling, fixes the details page layout hierarchy, and simplifies CMake configuration by enforcing a higher global minimum version and removing redundant per-directory declarations. Sequence diagram for unified device enable toggle via DeviceStatusItemsequenceDiagram
actor User
participant PageDevice as PageWiredOrWirelessDevice
participant DeviceStatusItem
participant D_Switch as D_Switch
participant dccData
participant NetManager
participant NetworkBackend
User->>PageDevice: Open device page
PageDevice->>DeviceStatusItem: Instantiate with netItem
activate DeviceStatusItem
DeviceStatusItem->>D_Switch: Bind checked to netItem.isEnabled
DeviceStatusItem->>D_Switch: Bind enabled to netItem.enabledable
deactivate DeviceStatusItem
User->>D_Switch: Click toggle
D_Switch->>DeviceStatusItem: onClicked()
alt netItem.isEnabled is true
DeviceStatusItem->>dccData: exec(NetManager.DisabledDevice, netItem.id, {})
else netItem.isEnabled is false
DeviceStatusItem->>dccData: exec(NetManager.EnabledDevice, netItem.id, {})
end
dccData->>NetManager: Execute enable/disable command
NetManager->>NetworkBackend: Apply device state change
NetworkBackend-->>NetManager: State updated
NetManager-->>PageDevice: netItem.status and isEnabled updated
PageDevice->>DeviceStatusItem: netItem property updated
DeviceStatusItem->>D_Switch: Update checked state
DeviceStatusItem->>DeviceStatusItem: Recompute status text via getStatusName()
Updated class diagram for QML components around device status and band configurationclassDiagram
class DeviceStatusItem {
<<QML_Component>>
+var netItem
+bool connectedNameVisible
+alias statusVisible
+getStatusName(status)
}
class PageWiredDevice {
<<QML_Page>>
+var netItem
+string name
+string parentName
+string displayName
+string icon
+int weight
+int pageType
}
class PageWirelessDevice {
<<QML_Page>>
+var netItem
+var airplaneItem
+var c_levelString
+string name
+string parentName
+string displayName
+string icon
+int weight
+int pageType
}
class SectionDevice {
<<QML_Section>>
+var config
+string parentName
+string type
+setConfig(config)
+getConfig() var
}
class SectionGeneric {
<<QML_Section>>
+var config
+string settingsID
+var band
+string errorKey
+editClicked()
}
class PageSettings {
<<QML_Page>>
+var config
+bool modified
+setConfig(config)
+saveConfig()
}
class PageDSLSettings {
<<QML_Page>>
+var config
+bool modified
+setConfig(config)
+saveConfig()
}
class NetUtils {
<<JS_Module>>
+numToIp(num)
+ipToNum(ip)
+strToByteArray(data)
-getStatusName(status) removed
}
PageWiredDevice *-- DeviceStatusItem : uses_as_main_page
PageWiredDevice *-- DeviceStatusItem : uses_in_details_item
PageWirelessDevice *-- DeviceStatusItem : uses_as_main_page
PageWirelessDevice *-- DeviceStatusItem : uses_in_details_item
PageSettings o-- SectionDevice : configures
PageSettings o-- SectionGeneric : configures
PageDSLSettings o-- SectionDevice : configures
PageDSLSettings o-- SectionGeneric : configures
SectionDevice ..> SectionGeneric : band_moved_out
NetUtils ..> DeviceStatusItem : getStatusName_moved_here
Flow diagram for wireless band configuration through SectionGenericflowchart LR
A_PageSettings["PageSettings.qml<br/>load existing connection config"] --> B_sectionDevice_setConfig["sectionDevice.setConfig(config[connection.type])"]
A_PageSettings --> C_set_band_to_SectionGeneric["sectionGeneric.band = config[connection.type].band"]
A_DSLSettings["PageDSLSettings.qml<br/>load DSL config"] --> D_sectionDevice_setConfig_dsl["sectionDevice.setConfig(config[802-3-ethernet])"]
A_DSLSettings --> E_set_band_to_SectionGeneric_dsl["sectionGeneric.band = config[802-3-ethernet].band"]
C_set_band_to_SectionGeneric --> F_SectionGeneric_band["SectionGeneric.qml<br/>property band"]
E_set_band_to_SectionGeneric_dsl --> F_SectionGeneric_band
F_SectionGeneric_band --> G_BandEditor["Band DccObject + ComboBox<br/>currentIndex = indexOfValue(root.band)"]
G_BandEditor --> H_User_selects_band["User selects Auto / 2.4GHz / 5GHz"]
H_User_selects_band --> I_update_band_property["onActivated: root.band = currentValue<br/>root.editClicked()"]
I_update_band_property --> J_save_from_PageSettings["PageSettings save handler"]
I_update_band_property --> K_save_from_PageDSLSettings["PageDSLSettings save handler"]
J_save_from_PageSettings --> L_devConfig_getConfig["devConfig = sectionDevice.getConfig()"]
L_devConfig_getConfig --> M_apply_band_normal["If sectionGeneric.band<br/>devConfig.band = sectionGeneric.band<br/>else delete devConfig.band"]
M_apply_band_normal --> N_write_back_normal["nConfig[connection.type] = devConfig"]
K_save_from_PageDSLSettings --> O_devConfig_getConfig_dsl["devConfig = sectionDevice.getConfig()"]
O_devConfig_getConfig_dsl --> P_apply_band_dsl["If sectionGeneric.band<br/>devConfig.band = sectionGeneric.band<br/>else delete devConfig.band"]
P_apply_band_dsl --> Q_write_back_dsl["nConfig[802-3-ethernet] = devConfig"]
Flow diagram for PageDetails layout restructuringflowchart LR
A_root_DccObject["PageDetails root DccObject"] --> B_body_DccObject_new["New body DccObject<br/>name = body<br/>parentName = root.name"]
B_body_DccObject_new --> C_DccRepeater["DccRepeater<br/>model NetItemModel"]
C_DccRepeater --> D_info_DccObject["Per item DccObject<br/>name = infoItem.name<br/>parentName = root.name + /body"]
D_info_DccObject --> E_title_DccObject["title DccObject<br/>RowLayout with label + copy icon"]
D_info_DccObject --> F_details_DccObject["details DccObject<br/>ColumnLayout of ItemDelegate rows"]
subgraph Previous_structure
G_old_DccRepeater["Old DccRepeater at root level"] --> H_old_info_DccObject["info DccObject children"]
H_old_info_DccObject --> I_old_title_DccObject
H_old_info_DccObject --> J_old_details_DccObject
K_old_body_placeholder["body DccObject declared after repeater"]
end
subgraph New_structure
B_body_DccObject_new
C_DccRepeater
D_info_DccObject
E_title_DccObject
F_details_DccObject
end
K_old_body_placeholder -.replaced_by.-> B_body_DccObject_new
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this 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 2 issues, and left some high level feedback:
- The new DeviceStatusItem logic for resolving the connected child name no longer filters on
itemType === NetType.WirelessItem(compared to the previous implementation in PageWirelessDevice), which may change which child is picked as the display name; consider reintroducing an explicit type check or documenting the intended behavior change. - There are new
console.warndebug statements left in PageSettings.qml arounddevConfig.interfaceNameandconnection.interface-name; if these are not meant for permanent logging, they should be removed or converted to the project’s standard logging mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new DeviceStatusItem logic for resolving the connected child name no longer filters on `itemType === NetType.WirelessItem` (compared to the previous implementation in PageWirelessDevice), which may change which child is picked as the display name; consider reintroducing an explicit type check or documenting the intended behavior change.
- There are new `console.warn` debug statements left in PageSettings.qml around `devConfig.interfaceName` and `connection.interface-name`; if these are not meant for permanent logging, they should be removed or converted to the project’s standard logging mechanism.
## Individual Comments
### Comment 1
<location> `dcc-network/qml/DeviceStatusItem.qml:49-57` </location>
<code_context>
+ var childrenItem = [netItem]
+ while (childrenItem.length > 0) {
+ var childItem = childrenItem.pop()
+ if (childItem.status === NetType.CS_Connected) {
+ return childItem.name
+ }
+ for (let i in childItem.children) {
+ childrenItem.push(childItem.children[i])
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Connected-name resolution now ignores item type, which may change behavior versus the previous implementation.
The previous logic required both `childItem.itemType === NetType.WirelessItem` and `childItem.status === NetType.CS_Connected`; now only `status` is checked. If non‑wireless children can also be `CS_Connected`, this may select a different (and possibly unintended) child name. If you want to preserve the prior behavior, please reintroduce an `itemType` constraint or otherwise narrow this search.
```suggestion
var childrenItem = [netItem]
while (childrenItem.length > 0) {
var childItem = childrenItem.pop()
if (childItem.itemType === NetType.WirelessItem &&
childItem.status === NetType.CS_Connected) {
return childItem.name
}
for (let i in childItem.children) {
childrenItem.push(childItem.children[i])
}
```
</issue_to_address>
### Comment 2
<location> `dcc-network/qml/PageSettings.qml:248-249` </location>
<code_context>
+ } else {
+ delete devConfig["band"]
+ }
+ console.warn("devConfig.interfaceName===================",devConfig.interfaceName)
+ console.warn("connection.interfaceName===================",nConfig["connection"]["interface-name"])
if (devConfig.interfaceName.length === 0) {
delete nConfig["connection"]["interface-name"]
</code_context>
<issue_to_address>
**suggestion:** Leftover `console.warn` debugging logs should be removed or gated.
These two calls appear to be temporary debug logging and will generate unnecessary noise in normal operation. Please either remove them or wrap them in a debug-only flag so they don’t run in production.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var childrenItem = [netItem] | ||
| while (childrenItem.length > 0) { | ||
| var childItem = childrenItem.pop() | ||
| if (childItem.status === NetType.CS_Connected) { | ||
| return childItem.name | ||
| } | ||
| for (let i in childItem.children) { | ||
| childrenItem.push(childItem.children[i]) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Connected-name resolution now ignores item type, which may change behavior versus the previous implementation.
The previous logic required both childItem.itemType === NetType.WirelessItem and childItem.status === NetType.CS_Connected; now only status is checked. If non‑wireless children can also be CS_Connected, this may select a different (and possibly unintended) child name. If you want to preserve the prior behavior, please reintroduce an itemType constraint or otherwise narrow this search.
| var childrenItem = [netItem] | |
| while (childrenItem.length > 0) { | |
| var childItem = childrenItem.pop() | |
| if (childItem.status === NetType.CS_Connected) { | |
| return childItem.name | |
| } | |
| for (let i in childItem.children) { | |
| childrenItem.push(childItem.children[i]) | |
| } | |
| var childrenItem = [netItem] | |
| while (childrenItem.length > 0) { | |
| var childItem = childrenItem.pop() | |
| if (childItem.itemType === NetType.WirelessItem && | |
| childItem.status === NetType.CS_Connected) { | |
| return childItem.name | |
| } | |
| for (let i in childItem.children) { | |
| childrenItem.push(childItem.children[i]) | |
| } |
Updated CMake minimum version requirement from 3.7 to 3.23 in main CMakeLists.txt Removed redundant cmake_minimum_required declarations from subdirectory CMakeLists files Added new DeviceStatusItem.qml component to centralize device status display logic Moved getStatusName function from NetUtils.js to DeviceStatusItem.qml for better encapsulation Refactored PageWiredDevice.qml and PageWirelessDevice.qml to use new DeviceStatusItem component Moved band configuration from SectionDevice.qml to SectionGeneric.qml for better organization Fixed PageDetails.qml layout structure by moving body container to proper position Removed debug console.log statements from NetUtils.js Log: Improved network device status display with unified component and better layout structure Influence: 1. Test wired and wireless device status display in network settings 2. Verify device enable/disable toggle functionality 3. Check band selection for wireless connections in network configuration 4. Test network details page layout and copy functionality 5. Verify DSL settings configuration with band parameter 6. Test network connection status display for various states (connected, disconnected, etc) feat: 重构网络设备状态显示和 CMake 版本 将主 CMakeLists.txt 中的 CMake 最低版本要求从 3.7 更新到 3.23 从子目录的 CMakeLists 文件中移除冗余的 cmake_minimum_required 声明 新增 DeviceStatusItem.qml 组件来集中管理设备状态显示逻辑 将 getStatusName 函数从 NetUtils.js 移至 DeviceStatusItem.qml 以实现更好 的封装 重构 PageWiredDevice.qml 和 PageWirelessDevice.qml 以使用新的 DeviceStatusItem 组件 将频段配置从 SectionDevice.qml 移至 SectionGeneric.qml 以优化组织结构 修复 PageDetails.qml 布局结构,将 body 容器移动到正确位置 移除 NetUtils.js 中的调试 console.log 语句 Log: 使用统一组件和更好的布局结构改进了网络设备状态显示 Influence: 1. 测试网络设置中有线和无线设备状态显示 2. 验证设备启用/禁用切换功能 3. 检查无线连接配置中的频段选择功能 4. 测试网络详情页面布局和复制功能 5. 验证包含频段参数的 DSL 设置配置 6. 测试各种状态(已连接、已断开等)的网络连接状态显示
deepin pr auto reviewGit Diff 代码审查报告1. 总体评价本次提交主要进行了以下几方面的修改:
整体来看,代码质量良好,重构方向合理,但有一些可以改进的地方。 2. 具体审查意见2.1 CMakeLists.txt 修改语法逻辑:
代码质量:
代码性能:
代码安全:
2.2 DeviceStatusItem.qml 新增文件语法逻辑:
代码质量:
代码性能:
代码安全:
2.3 NetUtils.js 修改语法逻辑:
代码质量:
代码性能:
代码安全:
2.4 PageDSLSettings.qml 和 PageSettings.qml 修改语法逻辑:
代码质量:
代码性能:
代码安全:
2.5 PageDetails.qml 修改语法逻辑:
代码质量:
代码性能:
代码安全:
2.6 PageWiredDevice.qml 和 PageWirelessDevice.qml 修改语法逻辑:
代码质量:
代码性能:
代码安全:
2.7 SectionDevice.qml 和 SectionGeneric.qml 修改语法逻辑:
代码质量:
代码性能:
代码安全:
3. 改进建议
4. 总结本次提交整体质量良好,主要进行了代码重构和优化,提高了代码的复用性和可维护性。主要改进点包括:
建议按照上述改进意见进行优化,以提高代码的健壮性和可维护性。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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 |
Updated CMake minimum version requirement from 3.7 to 3.23 in main CMakeLists.txt
Removed redundant cmake_minimum_required declarations from subdirectory CMakeLists files
Added new DeviceStatusItem.qml component to centralize device status display logic
Moved getStatusName function from NetUtils.js to DeviceStatusItem.qml for better encapsulation
Refactored PageWiredDevice.qml and PageWirelessDevice.qml to use new DeviceStatusItem component
Moved band configuration from SectionDevice.qml to SectionGeneric.qml for better organization
Fixed PageDetails.qml layout structure by moving body container to proper position
Removed debug console.log statements from NetUtils.js
Log: Improved network device status display with unified component and better layout structure
Influence:
feat: 重构网络设备状态显示和 CMake 版本
将主 CMakeLists.txt 中的 CMake 最低版本要求从 3.7 更新到 3.23
从子目录的 CMakeLists 文件中移除冗余的 cmake_minimum_required 声明 新增 DeviceStatusItem.qml 组件来集中管理设备状态显示逻辑
将 getStatusName 函数从 NetUtils.js 移至 DeviceStatusItem.qml 以实现更好 的封装
重构 PageWiredDevice.qml 和 PageWirelessDevice.qml 以使用新的 DeviceStatusItem 组件
将频段配置从 SectionDevice.qml 移至 SectionGeneric.qml 以优化组织结构 修复 PageDetails.qml 布局结构,将 body 容器移动到正确位置
移除 NetUtils.js 中的调试 console.log 语句
Log: 使用统一组件和更好的布局结构改进了网络设备状态显示
Influence:
Summary by Sourcery
Refactor network device status presentation and band configuration handling while updating the project’s CMake version requirements.
New Features:
Bug Fixes:
Enhancements:
Build: