fix: improve focus management for wheel scrolling in fullscreen #698
Conversation
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
1. Added scrolledByWheel property to track mouse wheel page navigation 2. Modified focus behavior to always focus first app when scrolling with wheel 3. Fixed inconsistent focus positioning after wheel vs keyboard navigation 4. Reset scrolledByWheel flag after processing to prevent stale state Log: Improved application focus behavior when using mouse wheel to navigate pages in fullscreen mode Influence: 1. Test mouse wheel page navigation in fullscreen mode 2. Verify focus correctly moves to first application after wheel scrolling 3. Compare focus behavior between wheel scrolling and keyboard navigation 4. Test edge cases like scrolling from first to last page and vice versa 5. Verify focus is not lost when switching between different navigation methods fix: 修复全屏模式下滚轮滚动时的焦点管理问题 1. 添加 scrolledByWheel 属性来跟踪鼠标滚轮翻页操作 2. 修改焦点行为，滚轮翻页时始终聚焦到第一个应用 3. 修复滚轮导航和键盘导航后焦点位置不一致的问题 4. 处理完成后重置 scrolledByWheel 标志，防止状态残留 Log: 改进了全屏模式下使用鼠标滚轮翻页时的应用焦点行为 Influence: 1. 测试全屏模式下的鼠标滚轮翻页功能 2. 验证滚轮翻页后焦点是否正确移动到第一个应用 3. 比较滚轮滚动和键盘导航的焦点行为差异 4. 测试边缘情况，如从第一页滚动到最后一页及反向操作 5. 验证在不同导航方法切换时焦点不会丢失 PMS: BUG-337493
deepin pr auto review这段代码主要是在处理通过鼠标滚轮翻页时的焦点逻辑变更。以下是对这段代码的审查意见，包括语法逻辑、代码质量、代码性能和代码安全方面的改进建议： 1. 语法逻辑优点：
改进建议：
2. 代码质量优点：
改进建议：
3. 代码性能优点：
改进建议：
4. 代码安全优点：
改进建议：
改进后的代码示例// 定义常量
readonly property int FIRST_PAGE_INDEX: 0
readonly property int LAST_PAGE_INDEX: listviewPage.count - 1
// 翻页逻辑
if (toPage < 0) {
flipPageDelay.start()
if (!searchEdit.focus) {
baseLayer.focus = true
}
handleWheelScroll(listviewPage, decrementPageIndex)
} else if (toPage > 0) {
flipPageDelay.start()
if (!searchEdit.focus) {
baseLayer.focus = true
}
handleWheelScroll(listviewPage, incrementPageIndex)
}
// 提取的滚轮翻页处理函数
function handleWheelScroll(listview, pageChangeFunc) {
listview.scrolledByWheel = true
pageChangeFunc(listview)
// 立即重置标志位，避免状态不一致
listview.scrolledByWheel = false
}
// onCurrentIndexChanged 中的逻辑
onCurrentIndexChanged: {
if (!listviewPage.visible) {
listviewPage.previousIndex = listviewPage.currentIndex
return
}
// 缓存属性值
const currentIndex = listviewPage.currentIndex
const previousIndex = listviewPage.previousIndex
if (listviewPage.scrolledByWheel) {
gridViewContainer.setPreviousPageSwitch(false)
listviewPage.scrolledByWheel = false
} else if (currentIndex + 1 === previousIndex || (previousIndex === FIRST_PAGE_INDEX && currentIndex === LAST_PAGE_INDEX)) {
gridViewContainer.setPreviousPageSwitch(true)
} else {
gridViewContainer.setPreviousPageSwitch(false)
}
}总结这段代码整体逻辑清晰，但可以通过减少代码重复、优化标志位生命周期管理和提取常量来提高代码质量和可维护性。性能和安全方面没有明显问题，但需要注意标志位的同步问题。 |
BLumia
approved these changes
Feb 2, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, wjyrich 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 GuideAdjusts fullscreen page navigation so that mouse wheel scrolling explicitly flags the navigation mode and ensures focus and page-switch state are handled consistently, always focusing the first app after a wheel-initiated page flip and resetting transient state afterward. Sequence diagram for fullscreen wheel vs keyboard page navigation focus handlingsequenceDiagram
actor User
participant InputEventItem
participant listviewPage
participant gridViewContainer
rect rgb(230,230,255)
User->>InputEventItem: wheelScroll(up or down)
InputEventItem->>listviewPage: scrolledByWheel = true
InputEventItem->>listviewPage: decrementPageIndex() or incrementPageIndex()
listviewPage-->>listviewPage: currentIndex changed
listviewPage->>gridViewContainer: setPreviousPageSwitch(false)
listviewPage->>listviewPage: scrolledByWheel = false
end
rect rgb(230,255,230)
User->>InputEventItem: keyboardNavigation
InputEventItem->>listviewPage: decrementPageIndex() or incrementPageIndex()
listviewPage-->>listviewPage: currentIndex changed
listviewPage->>gridViewContainer: setPreviousPageSwitch(true or false based on indices)
end
Class diagram for updated fullscreen listviewPage focus stateclassDiagram
class FullscreenFrame {
}
class InputEventItem {
+handleWheelScroll()
+decrementPageIndex(listviewPage)
+incrementPageIndex(listviewPage)
}
class listviewPage {
+int previousIndex
+bool scrolledByWheel
+int currentIndex
+int count
+onCurrentIndexChanged()
}
class gridViewContainer {
+setPreviousPageSwitch(isPreviousSwitch bool)
}
FullscreenFrame *-- InputEventItem
FullscreenFrame *-- listviewPage
FullscreenFrame *-- gridViewContainer
InputEventItem --> listviewPage : updatePageIndex
listviewPage --> gridViewContainer : setPreviousPageSwitch
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 1 issue, and left some high level feedback:
- Using a persistent
scrolledByWheelflag onlistviewPageto control the nextcurrentIndexchange makes the state a bit fragile; consider passing this context through the page flip function or via a dedicated signal/handler so the focus behavior is explicitly tied to a single navigation event instead of relying on a transient property that could be left true by future changes. - When
scrolledByWheelis true you currently skip thepreviousIndexcomparison logic entirely; if there are scenarios where wheel-triggered navigation should still respect the previous/next-page distinction, it may be clearer to factor the logic so the wheel-specific behavior (focus to first app) is applied in addition to, rather than instead of, thepreviousPageSwitchcalculation where appropriate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a persistent `scrolledByWheel` flag on `listviewPage` to control the next `currentIndex` change makes the state a bit fragile; consider passing this context through the page flip function or via a dedicated signal/handler so the focus behavior is explicitly tied to a single navigation event instead of relying on a transient property that could be left true by future changes.
- When `scrolledByWheel` is true you currently skip the `previousIndex` comparison logic entirely; if there are scenarios where wheel-triggered navigation should still respect the previous/next-page distinction, it may be clearer to factor the logic so the wheel-specific behavior (focus to first app) is applied in addition to, rather than instead of, the `previousPageSwitch` calculation where appropriate.
## Individual Comments
### Comment 1
<location> `qml/FullscreenFrame.qml:434` </location>
<code_context>
if (listviewPage.currentIndex === listviewPage.previousIndex) {
</code_context>
<issue_to_address>
**issue (bug_risk):** `scrolledByWheel` can remain stale when `currentIndex` doesn’t change, causing misclassification on the next index change.
In the early-return path where `currentIndex === previousIndex`, `scrolledByWheel` isn’t reset. If a wheel event sets `scrolledByWheel = true` but doesn’t change the index (e.g., trying to scroll past the first/last page), the flag leaks into the next index change and incorrectly marks it as wheel-driven. Please reset `listviewPage.scrolledByWheel` in this branch (or any short-circuit path) so the flag only applies to the wheel event that actually changes the index.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -431,7 +434,11 @@ InputEventItem { | |||
| listviewPage.previousIndex = listviewPage.currentIndex | |||
There was a problem hiding this comment.
issue (bug_risk): scrolledByWheel can remain stale when currentIndex doesn’t change, causing misclassification on the next index change.
In the early-return path where currentIndex === previousIndex, scrolledByWheel isn’t reset. If a wheel event sets scrolledByWheel = true but doesn’t change the index (e.g., trying to scroll past the first/last page), the flag leaks into the next index change and incorrectly marks it as wheel-driven. Please reset listviewPage.scrolledByWheel in this branch (or any short-circuit path) so the flag only applies to the wheel event that actually changes the index.
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: Improved application focus behavior when using mouse wheel to navigate pages in fullscreen mode
Influence:
fix: 修复全屏模式下滚轮滚动时的焦点管理问题
Log: 改进了全屏模式下使用鼠标滚轮翻页时的应用焦点行为
Influence:
PMS: BUG-337493
Summary by Sourcery
Improve focus management when navigating fullscreen pages with the mouse wheel so focus behavior matches keyboard navigation and remains consistent.
Bug Fixes: