fix: fix CustomComboBox modelData undefined errors#3107
Closed
robertkill wants to merge 1 commit intolinuxdeepin:masterfrom
Closed
fix: fix CustomComboBox modelData undefined errors#3107robertkill wants to merge 1 commit intolinuxdeepin:masterfrom
robertkill wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: robertkill 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCustomComboBox delegates in both power and personalization plugins now guard against undefined modelData via a safeModelData helper, provide empty-string fallbacks for text, use safeModelData for icon/enabled/visible role resolution, and switch the power plugin tooltip to the QtQuick.Controls attached ToolTip API with the necessary import. Sequence diagram for power plugin tooltip behavior with attached ToolTipsequenceDiagram
actor User
participant CustomComboBox
participant MenuItem
participant HoverHandler
participant ToolTip
User->>CustomComboBox: Open_popup
CustomComboBox->>MenuItem: Create_delegate_instances
User->>MenuItem: Move_mouse_over_item
MenuItem->>HoverHandler: Update_hover_state
HoverHandler-->>MenuItem: hovered_true
MenuItem->>MenuItem: Compute_fontMetrics_and_availableTextWidth
MenuItem->>ToolTip: Evaluate_visible_binding
ToolTip-->>ToolTip: visible_true_when_hovered_and_text_truncated
User->>MenuItem: Keep_mouse_over_item
ToolTip-->>User: Display_full_item_text
User->>MenuItem: Move_mouse_away
MenuItem->>HoverHandler: Update_hover_state
HoverHandler-->>MenuItem: hovered_false
MenuItem->>ToolTip: Reevaluate_visible_binding
ToolTip-->>ToolTip: visible_false
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review代码审查报告1. 语法逻辑改进点：
2. 代码质量改进点：
3. 代码性能改进点：
4. 代码安全改进点：
总结
以上修改可以显著提升代码的健壮性、可读性和性能。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
safeModelDatacan still benullwhenmodelDatais undefined, but you then accesssafeModelData[role](e.g., intext,icon.name,enabled,visible), which will throw at runtime; consider guarding withsafeModelData && safeModelData[role]or defaultingsafeModelDatato an empty object instead ofnullwhen property access is needed. - In the power plugin
CustomComboBoxdelegate you removedwidth: parent.width; unless this is intentional for layout changes, you may want to keep an explicit width binding to avoid the menu items shrinking to their implicit width.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `safeModelData` can still be `null` when `modelData` is undefined, but you then access `safeModelData[role]` (e.g., in `text`, `icon.name`, `enabled`, `visible`), which will throw at runtime; consider guarding with `safeModelData && safeModelData[role]` or defaulting `safeModelData` to an empty object instead of `null` when property access is needed.
- In the power plugin `CustomComboBox` delegate you removed `width: parent.width`; unless this is intentional for layout changes, you may want to keep an explicit width binding to avoid the menu items shrinking to their implicit width.
## Individual Comments
### Comment 1
<location path="src/plugin-power/qml/CustomComboBox.qml" line_range="20" />
<code_context>
- text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : model[control.textRole]) : modelData
+ readonly property var safeModelData: typeof modelData !== "undefined" ? modelData : null
+
+ text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : model[control.textRole]) : (safeModelData !== null ? safeModelData : "")
icon.name: (control.iconNameRole && model[control.iconNameRole] !== undefined) ? model[control.iconNameRole] : null
highlighted: control.highlightedIndex === index
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `safeModelData` being null before indexing into it.
If `Array.isArray(control.model)` is true while `typeof modelData === "undefined"`, `safeModelData` will be `null`, so `safeModelData[control.textRole]` will throw at runtime. You can guard the array branch similarly, for example:
```qml
text: control.textRole
? (Array.isArray(control.model)
? (safeModelData ? safeModelData[control.textRole] : "")
: model[control.textRole])
: (safeModelData !== null ? safeModelData : "")
```
so the new safety behavior applies to both object and array models.
</issue_to_address>
### Comment 2
<location path="src/plugin-personalization/qml/CustomComboBox.qml" line_range="16" />
<code_context>
useIndicatorPadding: true
- text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData
- icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : modelData[control.iconNameRole])) : null
+ text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : (model[control.textRole] === undefined ? safeModelData[control.textRole] : model[control.textRole])) : (safeModelData !== null ? safeModelData : "")
+ icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : safeModelData[control.iconNameRole])) : null
highlighted: control.highlightedIndex === index
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle the case where `safeModelData` is null in all branches before indexing.
When `modelData` is undefined, `safeModelData` is `null`, but the code still does `safeModelData[control.textRole]` in both branches, which will throw at runtime. Please guard all indexing with a null check on `safeModelData`, e.g.:
```qml
text: control.textRole
? (Array.isArray(control.model)
? (safeModelData ? safeModelData[control.textRole] : "")
: (model[control.textRole] === undefined
? (safeModelData ? safeModelData[control.textRole] : "")
: model[control.textRole]))
: (safeModelData !== null ? safeModelData : "")
```
and apply a similar check wherever `safeModelData[...]` is used.
</issue_to_address>
### Comment 3
<location path="src/plugin-personalization/qml/CustomComboBox.qml" line_range="17" />
<code_context>
- text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData
- icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : modelData[control.iconNameRole])) : null
+ text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : (model[control.textRole] === undefined ? safeModelData[control.textRole] : model[control.textRole])) : (safeModelData !== null ? safeModelData : "")
+ icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : safeModelData[control.iconNameRole])) : null
highlighted: control.highlightedIndex === index
hoverEnabled: control.hoverEnabled
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid indexing `safeModelData` for icon role when it may be null.
When `modelData` is undefined, `safeModelData` is `null`, but the `icon.name` fallback still indexes into it, which can cause a runtime error. Use a null-safe access, e.g.:
```qml
icon.name: control.iconNameRole
? (model[control.iconNameRole] !== undefined
? model[control.iconNameRole]
: (safeModelData ? safeModelData[control.iconNameRole] : null))
: null
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : model[control.textRole]) : modelData | ||
| readonly property var safeModelData: typeof modelData !== "undefined" ? modelData : null | ||
|
|
||
| text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : model[control.textRole]) : (safeModelData !== null ? safeModelData : "") |
There was a problem hiding this comment.
issue (bug_risk): Guard against safeModelData being null before indexing into it.
If Array.isArray(control.model) is true while typeof modelData === "undefined", safeModelData will be null, so safeModelData[control.textRole] will throw at runtime. You can guard the array branch similarly, for example:
text: control.textRole
? (Array.isArray(control.model)
? (safeModelData ? safeModelData[control.textRole] : "")
: model[control.textRole])
: (safeModelData !== null ? safeModelData : "")so the new safety behavior applies to both object and array models.
| useIndicatorPadding: true | ||
| text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData | ||
| icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : modelData[control.iconNameRole])) : null | ||
| text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : (model[control.textRole] === undefined ? safeModelData[control.textRole] : model[control.textRole])) : (safeModelData !== null ? safeModelData : "") |
There was a problem hiding this comment.
issue (bug_risk): Handle the case where safeModelData is null in all branches before indexing.
When modelData is undefined, safeModelData is null, but the code still does safeModelData[control.textRole] in both branches, which will throw at runtime. Please guard all indexing with a null check on safeModelData, e.g.:
text: control.textRole
? (Array.isArray(control.model)
? (safeModelData ? safeModelData[control.textRole] : "")
: (model[control.textRole] === undefined
? (safeModelData ? safeModelData[control.textRole] : "")
: model[control.textRole]))
: (safeModelData !== null ? safeModelData : "")and apply a similar check wherever safeModelData[...] is used.
| text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData | ||
| icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : modelData[control.iconNameRole])) : null | ||
| text: control.textRole ? (Array.isArray(control.model) ? safeModelData[control.textRole] : (model[control.textRole] === undefined ? safeModelData[control.textRole] : model[control.textRole])) : (safeModelData !== null ? safeModelData : "") | ||
| icon.name: control.iconNameRole ? ((model[control.iconNameRole] !== undefined ? model[control.iconNameRole] : safeModelData[control.iconNameRole])) : null |
There was a problem hiding this comment.
issue (bug_risk): Avoid indexing safeModelData for icon role when it may be null.
When modelData is undefined, safeModelData is null, but the icon.name fallback still indexes into it, which can cause a runtime error. Use a null-safe access, e.g.:
icon.name: control.iconNameRole
? (model[control.iconNameRole] !== undefined
? model[control.iconNameRole]
: (safeModelData ? safeModelData[control.iconNameRole] : null))
: null
18202781743
reviewed
Mar 20, 2026
| visible: hoverHandler.hovered && fontMetrics.advanceWidth(menuItem.text) > menuItem.availableTextWidth | ||
| text: menuItem.text | ||
| } | ||
| ToolTip.visible: hoverHandler.hovered && fontMetrics.advanceWidth(menuItem.text) > menuItem.availableTextWidth |
Contributor
There was a problem hiding this comment.
这个应该没啥区别吧，最终用的应该还是dtk的tooltip，
| useIndicatorPadding: true | ||
| width: parent.width | ||
| text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : model[control.textRole]) : modelData | ||
| readonly property var safeModelData: typeof modelData !== "undefined" ? modelData : null |
Contributor
There was a problem hiding this comment.
这个效果是一样的吧，undefined和null，
是不是下面应该去判断modelData有没有效，
1. Added safeModelData property to handle undefined modelData in both personalization and power plugins 2. Fixed text property to handle null safeModelData by providing empty string fallback 3. Fixed icon.name, enabled, and visible properties to use safeModelData instead of direct modelData access 4. In power plugin, replaced D.ToolTip with ToolTip attached property for better integration 5. Added QtQuick.Controls import for ToolTip support in power plugin Log: Fixed combobox display issues when modelData is undefined Influence: 1. Test comboboxes in personalization settings with various model types 2. Verify combobox items display correctly when modelData is undefined 3. Test tooltip functionality in power settings comboboxes 4. Verify icon display, enabled state, and visibility work properly with safeModelData 5. Test combobox behavior with both array and object-based models fix: 修复 CustomComboBox 中 modelData 未定义错误 1. 在个性化和电源插件中添加 safeModelData 属性来处理未定义的 modelData 2. 修复 text 属性，当 safeModelData 为 null 时提供空字符串回退 3. 修复 icon.name、enabled 和 visible 属性，使用 safeModelData 替代直接 访问 modelData 4. 在电源插件中，将 D.ToolTip 替换为 ToolTip 附加属性以获得更好的集成 5. 为电源插件添加 QtQuick.Controls 导入以支持 ToolTip Log: 修复了当 modelData 未定义时的组合框显示问题 Influence: 1. 测试个性化设置中的组合框，使用不同类型的模型 2. 验证当 modelData 未定义时组合框项目是否正确显示 3. 测试电源设置中组合框的工具提示功能 4. 验证图标显示、启用状态和可见性在使用 safeModelData 时正常工作 5. 测试组合框在数组和基于对象的模型下的行为
Contributor
Author
|
其它提交修复 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Log: Fixed combobox display issues when modelData is undefined
Influence:
fix: 修复 CustomComboBox 中 modelData 未定义错误
Log: 修复了当 modelData 未定义时的组合框显示问题
Influence:
Summary by Sourcery
Handle undefined modelData in CustomComboBox delegates to prevent display errors and improve tooltip integration in power and personalization plugins.
Bug Fixes:
Enhancements: