fix: Fix the issue where the theme area cannot switch focus#2901
Merged
Conversation
Reviewer's GuideRefactors the theme selection view and dock mode selector to be proper keyboard-focusable controls, introducing focus management, keyboard navigation, and activation handling while preserving existing mouse behavior and layout. Sequence diagram for keyboard focus and activation in theme selection viewsequenceDiagram
actor User
participant Root as FocusScope_root
participant ListView as ThemeListView
participant Model as GlobalThemeModel
participant Worker as ThemeWorker
User->>Root: Press Left/Right/Up/Down
Root->>Root: Update focusedThemeIndex
Root->>ListView: Update currentIndex if page changed
User->>Root: Press Enter/Return/Space
Root->>Root: activateCurrentItem()
Root->>Model: rowCount()
alt focusedThemeIndex < rowCount
Root->>Model: data(index(focusedThemeIndex, 0), IdRole)
Model-->>Root: themeId
Root->>ListView: selectTheme = themeId
Root->>Worker: setGlobalTheme(themeId)
else focusedThemeIndex == rowCount
Root->>Worker: goDownloadTheme()
end
Model-->>ListView: modelReset()
ListView->>Root: onModelReset handler
Root->>Root: Compare new rowCount with lastModelCount
alt rowCount changed
Root->>ListView: selectTheme = empty
Root->>Root: focusedThemeIndex = 0
Root->>Root: lastModelCount = newCount
else rowCount unchanged
Root->>Root: Keep current focusedThemeIndex
end
User->>Root: Focus moves away
Root->>Root: hadLostFocus = true
User->>Root: Focus returns
Root->>Root: If hadLostFocus, reset focusedThemeIndex to 0
Sequence diagram for dock mode selector keyboard navigation and activationsequenceDiagram
actor User
participant Repeater as ModeRepeater
participant Delegate as ModeItemDelegate
participant Dock as DockInter
User->>Delegate: Tab into first delegate
Delegate->>Delegate: activeFocusOnTab (index == 0)
User->>Delegate: Press Space or Return
Delegate->>Delegate: activate()
Delegate->>Dock: setDisplayMode(model.value)
User->>Delegate: Mouse click
Delegate->>Delegate: onClicked -> activate()
Delegate->>Dock: setDisplayMode(model.value)
User->>Delegate: Press Left
alt index > 0
Delegate->>Repeater: itemAt(index - 1)
Repeater-->>Delegate: previousItem
Delegate->>previousItem: children[0].forceActiveFocus()
else index == 0
Delegate->>Delegate: No action
end
User->>Delegate: Press Right
alt index < modeRepeater.count - 1
Delegate->>Repeater: itemAt(index + 1)
Repeater-->>Delegate: nextItem
Delegate->>nextItem: children[0].forceActiveFocus()
else index is last
Delegate->>Delegate: No action
end
Dock-->>Delegate: DisplayMode changes
Delegate->>Delegate: Border visible when DisplayMode == model.value or activeFocus
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 4 issues, and left some high level feedback:
- In ThemeSelectView.activateCurrentItem, using the hardcoded role value
0x201makes the code brittle; consider using a named role or a shared constant from the model instead of a magic number to retrieve the theme id. - The keyboard Up/Down handlers in ThemeSelectView update
focusedThemeIndexbut do not adjustlistview.currentIndex, which can cause the logical focus to move off the currently visible page; consider keepingcurrentIndexin sync withfocusedThemeIndexas you do for Left/Right. - In the dock
modeRepeaternavigation,modeRepeater.itemAt(index ± 1).children[0].forceActiveFocus()relies on implicit child ordering and could break if the delegate structure changes; it would be more robust to give the focusableItemDelegateanidand callforceActiveFocus()on that explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ThemeSelectView.activateCurrentItem, using the hardcoded role value `0x201` makes the code brittle; consider using a named role or a shared constant from the model instead of a magic number to retrieve the theme id.
- The keyboard Up/Down handlers in ThemeSelectView update `focusedThemeIndex` but do not adjust `listview.currentIndex`, which can cause the logical focus to move off the currently visible page; consider keeping `currentIndex` in sync with `focusedThemeIndex` as you do for Left/Right.
- In the dock `modeRepeater` navigation, `modeRepeater.itemAt(index ± 1).children[0].forceActiveFocus()` relies on implicit child ordering and could break if the delegate structure changes; it would be more robust to give the focusable `ItemDelegate` an `id` and call `forceActiveFocus()` on that explicitly.
## Individual Comments
### Comment 1
<location> `src/plugin-personalization/qml/ThemeSelectView.qml:21` </location>
<code_context>
- readonly property int imageRectH: 86
- readonly property int imageRectW: itemWidth
-
- property int gridMaxColumns: Math.floor(listview.width / (itemWidth + itemSpacing))
- property int gridMaxRows: 2
-
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against gridMaxColumns becoming 0 to avoid divide-by-zero and navigation issues.
When `listview.width` is 0 or very small, `gridMaxColumns` evaluates to 0, which is then used in `model: Math.ceil((rowCount + 1) / (2 * gridMaxColumns))` and in keyboard navigation. This can trigger divide-by-zero warnings and incorrect paging. Clamp the value to at least 1, e.g. `property int gridMaxColumns: Math.max(1, Math.floor(listview.width / (itemWidth + itemSpacing)))`.
</issue_to_address>
### Comment 2
<location> `src/plugin-personalization/qml/ThemeSelectView.qml:117-121` </location>
<code_context>
+ function onModelReset() {
+ var newCount = dccData.globalThemeModel.rowCount()
+ // 只有当模型数量真正发生变化时才重置焦点
+ if (newCount !== root.lastModelCount) {
+ listview.selectTheme = ""
+ root.focusedThemeIndex = 0
+ root.lastModelCount = newCount
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reset the ListView page index when the model changes to keep focus and page in sync.
On model reset you clear `selectTheme` and set `focusedThemeIndex` to 0, but leave `listview.currentIndex` unchanged. This can leave the view scrolled to a later page while focus is on the first item, so the focused theme isn’t visible until the user navigates. To keep focus and view aligned, also set `listview.currentIndex = 0` in this branch.
```suggestion
Connections {
target: dccData.globalThemeModel
function onModelReset() {
var newCount = dccData.globalThemeModel.rowCount()
// 只有当模型数量真正发生变化时才重置焦点
if (newCount !== root.lastModelCount) {
listview.selectTheme = ""
root.focusedThemeIndex = 0
listview.currentIndex = 0
root.lastModelCount = newCount
}
```
</issue_to_address>
### Comment 3
<location> `src/plugin-personalization/qml/ThemeSelectView.qml:83` </location>
<code_context>
+ function activateCurrentItem() {
+ var totalThemes = dccData.globalThemeModel.rowCount()
+ if (focusedThemeIndex < totalThemes) {
+ var themeId = dccData.globalThemeModel.data(dccData.globalThemeModel.index(focusedThemeIndex, 0), 0x201) // IdRole = Qt::UserRole + 0x101
+ listview.selectTheme = themeId
+ dccData.worker.setGlobalTheme(themeId)
</code_context>
<issue_to_address>
**suggestion:** Avoid using a hard-coded role number for the theme id; prefer a named role or constant.
Using the literal `0x201` tightly couples this code to the model’s internal role mapping and hurts maintainability. Prefer a named role from `roleNames()` (or a shared constant) so the code is self-describing and resilient to changes in role values.
Suggested implementation:
```
readonly property int themeIdRole: Qt.UserRole + 0x101 // IdRole
Keys.onReturnPressed: activateCurrentItem()
Keys.onEnterPressed: activateCurrentItem()
Keys.onSpacePressed: activateCurrentItem()
```
```
var themeId = dccData.globalThemeModel.data(dccData.globalThemeModel.index(focusedThemeIndex, 0), themeIdRole)
```
</issue_to_address>
### Comment 4
<location> `src/plugin-dock/qml/main.qml:96-98` </location>
<code_context>
+ Keys.onSpacePressed: activate()
+ Keys.onReturnPressed: activate()
+
+ Keys.onLeftPressed: {
+ if (index > 0) {
+ modeRepeater.itemAt(index - 1).children[0].forceActiveFocus()
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid relying on `children[0]` to access the delegate when moving focus between items.
`modeRepeater.itemAt(index - 1)` returns the `ColumnLayout`, and this assumes its first child is always the `D.ItemDelegate`. Any change to the layout’s child structure (e.g. wrapping the delegate) will break focus navigation. Prefer making `D.ItemDelegate` the root delegate, or give it an `objectName` and resolve it via `itemAt(...).findChild(...)` instead of relying on child order.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| function incrementCurrentIndex() { | ||
| listview.incrementCurrentIndex() | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Guard against gridMaxColumns becoming 0 to avoid divide-by-zero and navigation issues.
When listview.width is 0 or very small, gridMaxColumns evaluates to 0, which is then used in model: Math.ceil((rowCount + 1) / (2 * gridMaxColumns)) and in keyboard navigation. This can trigger divide-by-zero warnings and incorrect paging. Clamp the value to at least 1, e.g. property int gridMaxColumns: Math.max(1, Math.floor(listview.width / (itemWidth + itemSpacing))).
Comment on lines
+117
to
+121
| Connections { | ||
| target: dccData.globalThemeModel | ||
| function onModelReset() { | ||
| var newCount = dccData.globalThemeModel.rowCount() | ||
| // 只有当模型数量真正发生变化时才重置焦点 |
There was a problem hiding this comment.
suggestion (bug_risk): Reset the ListView page index when the model changes to keep focus and page in sync.
On model reset you clear selectTheme and set focusedThemeIndex to 0, but leave listview.currentIndex unchanged. This can leave the view scrolled to a later page while focus is on the first item, so the focused theme isn’t visible until the user navigates. To keep focus and view aligned, also set listview.currentIndex = 0 in this branch.
Suggested change
| Connections { | |
| target: dccData.globalThemeModel | |
| function onModelReset() { | |
| var newCount = dccData.globalThemeModel.rowCount() | |
| // 只有当模型数量真正发生变化时才重置焦点 | |
| Connections { | |
| target: dccData.globalThemeModel | |
| function onModelReset() { | |
| var newCount = dccData.globalThemeModel.rowCount() | |
| // 只有当模型数量真正发生变化时才重置焦点 | |
| if (newCount !== root.lastModelCount) { | |
| listview.selectTheme = "" | |
| root.focusedThemeIndex = 0 | |
| listview.currentIndex = 0 | |
| root.lastModelCount = newCount | |
| } |
| function activateCurrentItem() { | ||
| var totalThemes = dccData.globalThemeModel.rowCount() | ||
| if (focusedThemeIndex < totalThemes) { | ||
| var themeId = dccData.globalThemeModel.data(dccData.globalThemeModel.index(focusedThemeIndex, 0), 0x201) // IdRole = Qt::UserRole + 0x101 |
There was a problem hiding this comment.
suggestion: Avoid using a hard-coded role number for the theme id; prefer a named role or constant.
Using the literal 0x201 tightly couples this code to the model’s internal role mapping and hurts maintainability. Prefer a named role from roleNames() (or a shared constant) so the code is self-describing and resilient to changes in role values.
Suggested implementation:
readonly property int themeIdRole: Qt.UserRole + 0x101 // IdRole
Keys.onReturnPressed: activateCurrentItem()
Keys.onEnterPressed: activateCurrentItem()
Keys.onSpacePressed: activateCurrentItem()
var themeId = dccData.globalThemeModel.data(dccData.globalThemeModel.index(focusedThemeIndex, 0), themeIdRole)
pengfeixx
force-pushed
the
fix-311337
branch
from
e1dabd5 to
398bf58
Compare
Fix the issue where the theme area cannot switch focus Log: Fix the issue where the theme area cannot switch focus pms: BUG-311337
pengfeixx
force-pushed
the
fix-311337
branch
from
398bf58 to
dcf8b37
Compare
deepin pr auto review我来对这段代码进行详细审查：
main.qml:
ThemeSelectView.qml:
main.qml:
ThemeSelectView.qml:
main.qml:
ThemeSelectView.qml:
main.qml:
ThemeSelectView.qml:
总体来说，这些改动提高了代码的可访问性和用户体验，特别是在键盘导航方面。代码结构清晰，逻辑合理，但还可以在性能优化和错误处理方面做进一步改进。 |
caixr23
approved these changes
Dec 24, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, pengfeixx 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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.
Fix the issue where the theme area cannot switch focus
Log: Fix the issue where the theme area cannot switch focus
pms: BUG-311337
Summary by Sourcery
Improve keyboard focus handling for theme selection and dock display mode selection views.
Bug Fixes:
Enhancements: