Repository navigation
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Reviewer's GuideThe PR fixes dropdown sizing by measuring realized visible delegates for intrinsic width and row height, using those measurements to calculate list geometry and scrolling, and removing the delegate’s dependency on the popup width so the popup can grow to fit its content. Sequence diagram for dropdown content measurement and sizingsequenceDiagram
participant ComboBox
participant Popup
participant ArrowListView
participant ListView
participant Delegate
ComboBox->>Popup: implicitWidth
Popup->>ArrowListView: sizedHeight
ArrowListView->>ListView: contentHeight
ListView->>Delegate: itemAtIndex(i)
Delegate-->>ArrowListView: visible height and implicitWidth
ArrowListView->>ArrowListView: calculate rowHeight and itemMaxWidth
ArrowListView-->>Popup: implicitWidth and implicitHeight
Popup-->>ComboBox: sized dropdown geometry
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="qt6/src/qml/ArrowListView.qml" line_range="20-26" />
<code_context>
- // Full height of the list, to size a popup before it is mapped.
+ // Height of a visible realized row, or the style height before one exists:
+ // row 0 can be a row hidden by a visible role, and its height is 0.
+ readonly property real rowHeight: {
+ if (itemsView.contentHeight > 0) {
+ for (var i = 0; i < itemsView.count; ++i) {
+ const item = itemsView.itemAtIndex(i)
+ if (item && item.visible && item.height > 0)
+ return item.height
+ }
+ }
+ return itemHeight
</code_context>
<issue_to_address>
**Mixed row heights mis-size the menu**
When visible delegates have different heights and the first realized visible row is not representative of the first `maxVisibleItems` rows, `rowHeight` uses that first row, and `implicitHeight` multiplies it by `maxVisibleItems`. A short first row clips later taller rows, while a tall first row can leave room for too many shorter rows and keep `interactive` false; users see fewer or more rows than intended, without the expected scroll controls.
Size the viewport using the summed heights of the first `maxVisibleItems` visible rows, so the height cap and scrolling threshold reflect the actual rows.
Also at `qt6/src/qml/ArrowListView.qml:66-67`.
</issue_to_address>
### Comment 2
<location path="qt6/src/qml/ComboBox.qml" line_range="177" />
<code_context>
rightMargin: DS.Style.popup.margin
palette: control.palette
- implicitWidth: control.flat ? Math.max(contentItem.implicitWidth, control.width) : control.width
+ implicitWidth: control.flat ? Math.max(contentItem.implicitWidth + leftMargin + rightMargin, control.width) : control.width
// 60 is a temporary viewport so the list can realize one row and be measured.
implicitHeight: (contentItem.sizedHeight > 0 ? contentItem.sizedHeight : 2 * DS.Style.arrowListView.itemHeight)
</code_context>
<issue_to_address>
**Popup width uses placement margins**
When a flat ComboBox has content wider than its control and the popup margin differs from its horizontal content padding, the `implicitWidth` formula adds `leftMargin` and `rightMargin`, which constrain window-edge placement rather than pad content. The popup becomes wider than needed when margins exceed padding, or clips content when they are smaller.
Use the popup's horizontal content padding, not its placement margins, in the width calculation.
</issue_to_address>
### Comment 3
<location path="qt6/src/qml/ArrowListView.qml" line_range="39" />
<code_context>
const arrowStrips = itemsView.count > maxVisibleItems
? 2 * (DS.Style.arrowListView.stepButtonIconSize.height + contentLayout.spacing) : 0
- return Math.min(itemsView.count, maxVisibleItems) * (measured > 0 ? measured : itemHeight) + arrowStrips
+ return Math.min(itemsView.count, maxVisibleItems) * rowHeight + arrowStrips
}
</code_context>
<issue_to_address>
**Hidden rows leave blank popup**
When the model has items but every delegate is hidden, `sizedHeight` falls back to `count * rowHeight` when `contentHeight` is zero, so an all-hidden model still opens a nonzero-height blank popup.
Return zero once layout confirms there are no visible rows, while retaining the estimate only before rows have been realized.
</issue_to_address>
mhduiy
force-pushed
the
bug-377521-dropdown-sizing
branch
2 times, most recently
from
October 15, 2026 05:46
3f77146 to
8403c94
Compare
18202781743
reviewed
Oct 15, 2026
| if (!item || !item.visible) | ||
| continue | ||
| item.width = itemsView.width | ||
| if (item.implicitWidth > itemMaxWidth) |
Contributor
There was a problem hiding this comment.
itemMaxWidth可能在这个循环里触发多次，可以用临时变量存储中间值，
18202781743
reviewed
Oct 15, 2026
| } | ||
| interactive: itemsView.count > maxVisibleItems | ||
| implicitHeight: itemsView.count > 0 ? Math.min(contentHeight, maxVisibleItems * control.rowHeight) : 0 | ||
| implicitWidth: itemMaxWidth |
Contributor
There was a problem hiding this comment.
implicitWidth重构与原来的逻辑是一样的吧，只是多了个visible的判断么？
Contributor
Author
There was a problem hiding this comment.
算是，重复逻辑都抽出到refreshContentItemWidth中，
另外就是主要是修复了一个循环依赖，popup, menuitem 相互取宽度，导致最终算出来的宽度是错误的。
删除 implicitWidth: Math.max(DS.Style.control.implicitWidth(control), popup.implicitWidth - DS.Style.popup.margin * 2)
18202781743
reviewed
Oct 15, 2026
| // Full height of the list, to size a popup before it is mapped. | ||
| // Height of a visible realized row, or the style height before one exists: | ||
| // row 0 can be a row hidden by a visible role, and its height is 0. | ||
| readonly property real rowHeight: { |
Contributor
There was a problem hiding this comment.
rowHeight不暴露出去吧？加上下划线标识私有变量，
1. Measure realized visible items and retain their maximum width. 2. Use visible row height and laid-out content to size the list. 3. Keep ComboBox delegate implicit width independent of popup width. Log: Correct dropdown width measurement and height sizing Influence: Improve dropdown sizing and item visibility. fix(qml): 修正下拉列表尺寸计算 1. 测量已创建的可见项并保留最大宽度。 2. 使用可见项行高和布局后的内容计算列表高度。 3. 移除组合框委托隐式宽度对弹窗宽度的依赖。 Log: 修正下拉列表宽度测量和高度计算 PMS: BUG-377521 Influence: 改善下拉列表尺寸和选项显示。
mhduiy
force-pushed
the
bug-377521-dropdown-sizing
branch
from
October 15, 2026 08:22
8403c94 to
47eefcd
Compare
Contributor
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 语法正确，逻辑清晰，无需修改。建议在sizedHeight中当contentHeight > 0时返回implicitHeight处添加注释说明此时arrowStrips已包含在布局高度中。 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 代码结构清晰，注释完整。建议为itemMaxWidth属性和refreshContentItemWidth中的缓存逻辑添加简短注释，提升可维护性。 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 性能良好，资源使用合理。对于大数据量列表场景，可考虑将_rowHeight的计算结果缓存，避免每次binding重新求值时遍历。 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： 存在0个安全漏洞。QML UI代码无安全风险，无需安全加固。 💡 改进建议代码示例// ArrowListView.qml - 建议为itemMaxWidth添加注释
// 缓存已测量项的最大隐式宽度，避免每次从零计算
property real itemMaxWidth: 0
// ComboBox.qml - 当前实现已正确修复循环依赖问题
// delegate不再依赖popup.implicitWidth，消除循环依赖
delegate: MenuItem {
useIndicatorPadding: true
// implicitWidth已移除，由refreshContentItemWidth统一测量
}本报告由 AI 代码审查工具自动生成 |
Contributor
|
TAG Bot New tag: 6.7.51 |
This branch has not been deployed
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
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.
PMS: BUG-377521
Summary
Verification
cmake --build build6 -j8Summary by Sourcery
Fix QML dropdown sizing so popup dimensions accurately reflect visible rows and content.
Bug Fixes:
Enhancements: