fix(ComboBox): fix highlight lingering after mouse leaves#585
fix(ComboBox): fix highlight lingering after mouse leaves#585deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideComboBox menu item highlighting is now gated on a new popup-level hover state so that the highlighted style clears when the mouse leaves the popup list area, implemented separately for Qt5 (MouseArea) and Qt6 (HoverHandler). Class diagram for updated ComboBox popup and delegate highlighting (Qt5 and Qt6)classDiagram
class ComboBox {
int currentIndex
int highlightedIndex
bool hoverEnabled
Popup popup
}
class Popup {
bool contentHovered
int implicitWidth
ArrowListView contentItem
}
class ArrowListView {
int maxVisibleItems
ListView view
}
class ListView {
int currentIndex
int highlightRangeMode
int highlightMoveDuration
}
class DelegateItem {
bool highlighted
bool hoverEnabled
bool autoExclusive
bool checked
+updateHighlighted(popupContentHovered, controlHighlightedIndex, index)
}
class MouseArea_Qt5 {
bool hoverEnabled
int acceptedButtons
+onContainsMouseChanged(containsMouse)
}
class HoverHandler_Qt6 {
+onHoveredChanged(hovered)
}
ComboBox --> Popup : owns
Popup --> ArrowListView : contentItem
ArrowListView --> ListView : view
ArrowListView --> MouseArea_Qt5 : Qt5 only
ArrowListView --> HoverHandler_Qt6 : Qt6 only
ListView --> DelegateItem : provides delegates
DelegateItem : highlighted = popup.contentHovered && control.highlightedIndex === index
MouseArea_Qt5 : onContainsMouseChanged sets popup.contentHovered
HoverHandler_Qt6 : onHoveredChanged sets popup.contentHovered
Flow diagram for ComboBox popup hover state driving delegate.highlightedflowchart TD
A[Mouse enters menu item area] --> B[Popup.contentHovered set to true]
B --> C[Control.highlightedIndex updated for hovered item]
C --> D[Delegate.highlighted evaluates popup.contentHovered && control.highlightedIndex === index]
D --> E[Matching delegate shows highlighted style]
F[Mouse leaves popup list area] --> G[Popup.contentHovered set to false]
G --> H[Delegate.highlighted re-evaluates to false for all items]
H --> I[All menu items clear highlighted style]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In both Qt5 and Qt6 versions, tying
delegate.highlightedtopopup.contentHovered && control.highlightedIndex === indexmeans keyboard-based navigation will no longer visually highlight items when the mouse is outside the popup; consider whethercontentHoveredshould be ignored (or handled differently) for focus/keyboard navigation to preserve accessibility and expected behavior. - For the Qt6
HoverHandler, instead of imperatively settingpopup.contentHoveredinonHoveredChanged, consider bindingpopup.contentHovereddirectly to the handler’shoveredproperty to avoid potential desynchronization if additional hover logic is added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both Qt5 and Qt6 versions, tying `delegate.highlighted` to `popup.contentHovered && control.highlightedIndex === index` means keyboard-based navigation will no longer visually highlight items when the mouse is outside the popup; consider whether `contentHovered` should be ignored (or handled differently) for focus/keyboard navigation to preserve accessibility and expected behavior.
- For the Qt6 `HoverHandler`, instead of imperatively setting `popup.contentHovered` in `onHoveredChanged`, consider binding `popup.contentHovered` directly to the handler’s `hovered` property to avoid potential desynchronization if additional hover logic is added later.
## Individual Comments
### Comment 1
<location path="qt6/src/qml/ComboBox.qml" line_range="39-42" />
<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] : null
- highlighted: control.highlightedIndex === index
+ highlighted: popup.contentHovered && control.highlightedIndex === index
hoverEnabled: control.hoverEnabled
autoExclusive: true
</code_context>
<issue_to_address>
**issue (bug_risk):** In Qt 6, `popup` is both missing an `id` and not visible from the delegate context.
`popup.contentHovered` will always fail here: the `Popup` has no `id`, so `popup` is undefined in this file, and even with `id: popup` the delegate can’t see it because the popup isn’t in the delegate’s context. Instead, expose the hover state via something the delegate can access (for example, a `control.popupContentHovered` property updated by the popup) and bind `highlighted` to that property.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
e136cd3 to
42dda9f
Compare
qt6/src/qml/ComboBox.qml
Outdated
| 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] : null | ||
| highlighted: control.highlightedIndex === index | ||
| highlighted: popup.contentHovered && control.highlightedIndex === index |
There was a problem hiding this comment.
popup是可以从外部赋值的,不应该在ComboBox里访问popup,可以参照 https://github.com/linuxdeepin/dtkdeclarative/pull/569/changes
5edc15e to
8398b4e
Compare
qt6/src/qml/ComboBox.qml
Outdated
| view.highlightRangeMode: ListView.ApplyRange | ||
| view.highlightMoveDuration: 0 | ||
| Keys.priority: Keys.BeforeItem | ||
| Keys.onPressed: (event) => { |
There was a problem hiding this comment.
这是手搓了个ComboBox的按键处理呀,无法借助qt里面的处理么,
|
TAG Bot New tag: 6.7.36 |
8398b4e to
561f621
Compare
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
7766bca to
dd67e9e
Compare
dd67e9e to
42fd9ed
Compare
- Track the popup's hover state by adding a isInteractingWithContent property to ComboBox, and modify the highlighted property in the delegate. - The logic of the highlighted property now depends on isInteractingWithContent, automatically clearing the highlight when the mouse leaves. 修复鼠标移出后高亮项仍然残留的问题 - 在ComboBox上添加isInteractingWithContent属性跟踪popup悬停状态,修改delegate的highlighted属性 - highlighted属性逻辑依赖isInteractingWithContent,鼠标离开时自动清除高亮 Log: 修复ComboBox的菜单项鼠标离开后高亮残留问题 Influence: ComboBox菜单项 PMS: BUG-304991
42fd9ed to
33d6d98
Compare
deepin pr auto review这段代码主要在 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
5. 改进建议为了解决上述逻辑冲突和状态管理问题,建议进行以下修改: 方案 A:优化状态逻辑(推荐) 明确交互的定义,避免鼠标移出时立即清除状态。可以将状态分为“键盘交互中”和“鼠标交互中”,或者仅在特定条件下清除状态。 // ... 其他代码
property bool isInteractingWithContent: false
// ... 其他代码
popup: T.Popup {
// ... 其他配置
onClosed: control.isInteractingWithContent = false
Connections {
target: control
function onHighlightedIndexChanged() {
// 键盘导航时保持交互状态
if (control.highlightedIndex >= 0)
control.isInteractingWithContent = true
}
function onActiveFocusChanged() {
// 如果控件失去焦点,通常意味着交互结束
if (!control.activeFocus) {
control.isInteractingWithContent = false
}
}
}
HoverHandler {
// 使用 target 指定具体的 ListView 或内容区域,而不是整个 Popup
// 这样可以避免在 Popup 的边距区域移动鼠标时触发状态变化
id: hoverHandler
onHoveredChanged: {
// 仅当鼠标悬停在内容上时才更新状态
// 并且不直接设置为 false,以免覆盖键盘操作的状态
if (hovered) {
control.isInteractingWithContent = true
}
// 注意:这里移除了 !hovered 时设为 false 的逻辑
// 依靠 onClosed 或 onActiveFocusChanged 来重置
}
}
// ... 其他代码
}方案 B:使用状态机(复杂场景) 如果交互逻辑非常复杂,建议使用 QML 的 总结这段代码的初衷是为了改善高亮显示的交互体验,但目前的实现方式在鼠标和键盘混合操作时可能存在逻辑冲突。建议优化状态更新的逻辑,确保 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, electricface 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 |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
and modify the highlighted property in the delegate.
isInteractingWithContent, automatically clearing the highlight when the
mouse leaves.
修复鼠标移出后高亮项仍然残留的问题
Log: 修复ComboBox的菜单项鼠标离开后高亮残留问题
Influence: ComboBox菜单项
PMS: BUG-304991