fix: fix warning with closebtn and binding loop.#1517
Merged
18202781743 merged 1 commit intolinuxdeepin:masterfrom Mar 20, 2026
Merged
fix: fix warning with closebtn and binding loop.#151718202781743 merged 1 commit intolinuxdeepin:masterfrom
18202781743 merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideAdjusts notification focus and close-button behavior to eliminate binding loops and ensure predictable Tab/Shift+Tab navigation across notification items, including overlap and normal notifications, while simplifying how clear buttons are referenced and activated. Sequence diagram for Tab navigation within a notification itemsequenceDiagram
actor User
participant NotifyItem as NotifyItem(OverlapOrNormal)
participant NotifyContent as NotifyItemContent
participant Buttons as ActionButtons
participant NextItem as NextNotifyItem
User->>NotifyItem: Press Tab
activate NotifyItem
NotifyItem->>NotifyContent: Keys.onTabPressed
activate NotifyContent
NotifyContent->>Buttons: focusFirstActionOnly()
alt Has_action_buttons
Buttons-->>NotifyContent: true (focus moved to first action)
NotifyContent-->>NotifyItem: event.accepted = true
else No_action_buttons
Buttons-->>NotifyContent: false
NotifyContent-->>NotifyItem: delegate.gotoNextItem()
deactivate NotifyContent
NotifyItem->>NextItem: gotoNextItem signal
activate NextItem
NextItem->>NextItem: shouldShowClose = true (if defined)
NextItem->>NextItem: forceActiveFocus()
deactivate NextItem
end
deactivate NotifyItem
Sequence diagram for Shift+Tab navigation across notification itemssequenceDiagram
actor User
participant NotifyItem as NotifyItem(OverlapOrNormal)
participant NotifyContent as NotifyItemContent
participant Buttons as Buttons
participant PrevItem as PrevNotifyItem
User->>NotifyItem: Press Shift+Tab
activate NotifyItem
NotifyItem->>NotifyItem: root.shouldShowClose = true
NotifyItem->>NotifyContent: Keys.onBacktabPressed
activate NotifyContent
NotifyContent->>Buttons: focusLastButton()
alt Has_focusable_button
Buttons-->>NotifyContent: true (focus moved to last button)
NotifyContent-->>NotifyItem: event.accepted = true
else No_focusable_button
Buttons-->>NotifyContent: false
NotifyContent-->>NotifyItem: delegate.gotoPrevItem()
deactivate NotifyContent
NotifyItem->>PrevItem: gotoPrevItem signal
activate PrevItem
PrevItem->>PrevItem: shouldShowClose = true (if defined)
PrevItem->>PrevItem: forceActiveFocus()
deactivate PrevItem
end
deactivate NotifyItem
Sequence diagram for initial focus when opening notification centersequenceDiagram
participant NotifyView as NotifyView
participant ViewList as NotificationListView
participant Item as NotifyItemDelegate
participant Buttons as Buttons
NotifyView->>NotifyView: tryFocus(retries)
NotifyView->>ViewList: itemAtIndex(idx)
ViewList-->>NotifyView: Item
alt Item_exists_and_enabled
NotifyView->>Item: check focusFirstButton
alt Item_has_focusFirstButton_and_succeeds
Item->>Buttons: focusFirstButton()
Buttons-->>Item: true (button focused)
else No_focusFirstButton_or_failed
Item-->>NotifyView: false
NotifyView->>Item: forceActiveFocus()
end
else Item_missing_or_disabled
NotifyView->>NotifyView: if retries > 0 callLater(tryFocus)
end
Class diagram for updated notification focus componentsclassDiagram
class NotifyView {
+function tryFocus(retries)
-int idx
-NotificationListView view
}
class NotifyViewDelegate {
+bool shouldShowClose
+function gotoNextItem()
+function gotoPrevItem()
}
class NotifyItemContent {
+readonly int maxFocusRetries
+bool parentHovered
+bool closeVisible
+int miniContentHeight
+bool enableDismissed
+alias clearButton
+readonly alias clearButtonItem
+signal gotoNextItem()
+signal gotoPrevItem()
+function focusFirstActionOnly()
+function focusLastButton()
}
class OverlapNotify {
+signal gotoNextItem()
+signal gotoPrevItem()
+var clearButton
+bool shouldShowClose
+function focusFirstButton()
}
class NormalNotify {
+signal gotoNextItem()
+signal gotoPrevItem()
+bool shouldShowClose
+function focusFirstButton()
}
class GroupNotify {
+signal gotoNextItem()
+Button groupClearBtn
}
class NotifyHeader {
+signal gotoFirstNotify()
+Button clearAllBtn
}
class ActionButtons {
+function focusFirstActionOnly()
+function focusLastButton()
}
NotifyView --> NotifyViewDelegate : uses
NotifyViewDelegate --> OverlapNotify : delegates_to
NotifyViewDelegate --> NormalNotify : delegates_to
OverlapNotify o-- NotifyItemContent : contains
NormalNotify o-- NotifyItemContent : contains
NotifyItemContent --> ActionButtons : manages_focus_for
OverlapNotify --> ActionButtons : focusFirstButton()
NormalNotify --> ActionButtons : focusFirstButton()
OverlapNotify --> NotifyViewDelegate : gotoNextItem/gotoPrevItem
NormalNotify --> NotifyViewDelegate : gotoNextItem/gotoPrevItem
GroupNotify --> NotifyViewDelegate : gotoNextItem
NotifyHeader --> NotifyViewDelegate : gotoFirstNotify
OverlapNotify --> NotifyItemContent : clearButton = clearButtonItem
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:
- The repeated Backtab handling logic in NormalNotify and OverlapNotify (toggling shouldShowClose and preferring notifyContent.focusLastButton before gotoPrevItem) could be extracted into a shared helper to keep the focus behavior consistent and easier to maintain.
- The pattern
if (nextItem.shouldShowClose !== undefined)/if (prevItem.shouldShowClose !== undefined)is a bit brittle; consider exposing a small method (e.g.ensureCloseVisible()) on the delegate instead of reaching into an optional property from the outside.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated Backtab handling logic in NormalNotify and OverlapNotify (toggling shouldShowClose and preferring notifyContent.focusLastButton before gotoPrevItem) could be extracted into a shared helper to keep the focus behavior consistent and easier to maintain.
- The pattern `if (nextItem.shouldShowClose !== undefined)` / `if (prevItem.shouldShowClose !== undefined)` is a bit brittle; consider exposing a small method (e.g. `ensureCloseVisible()`) on the delegate instead of reaching into an optional property from the outside.
## Individual Comments
### Comment 1
<location path="panels/notification/plugin/NotifyItemContent.qml" line_range="19-26" />
<code_context>
readonly property int maxFocusRetries: 5
property bool parentHovered: false // External hover state from parent component
- property bool closeVisible: activeFocus || impl.hovered || parentHovered || (clearLoader.item && clearLoader.item.activeFocus)
+ property bool closeVisible: activeFocus || impl.hovered || parentHovered
property int miniContentHeight: NotifyStyle.contentItem.miniHeight
property bool enableDismissed: true
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-evaluate visibility/activation conditions now that clear button focus is no longer considered explicitly.
Previously, `closeVisible` and `clearLoader.active` stayed true while the clear button itself had focus by checking `clearLoader.item && clearLoader.item.activeFocus`. Now `closeVisible` relies only on `root.activeFocus` and hover. In many QML hierarchies only the focused child has `activeFocus`, so when the close button gains focus, `root.activeFocus` may become false, hiding/unloading the button while it is still focused. Please verify whether `root` keeps `activeFocus` when the close button is focused; if not, consider reintroducing a focus check tied directly to the clear/close button (e.g., its `activeFocus`) in these conditions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+19
to
26
| property bool closeVisible: activeFocus || impl.hovered || parentHovered | ||
| property int miniContentHeight: NotifyStyle.contentItem.miniHeight | ||
| property bool enableDismissed: true | ||
| property alias clearButton: clearLoader.sourceComponent | ||
| readonly property alias clearButtonItem: clearLoader.item | ||
|
|
||
| signal gotoNextItem() // Signal to navigate to next notify item | ||
| signal gotoPrevItem() // Signal to navigate to previous notify item |
There was a problem hiding this comment.
issue (bug_risk): Re-evaluate visibility/activation conditions now that clear button focus is no longer considered explicitly.
Previously, closeVisible and clearLoader.active stayed true while the clear button itself had focus by checking clearLoader.item && clearLoader.item.activeFocus. Now closeVisible relies only on root.activeFocus and hover. In many QML hierarchies only the focused child has activeFocus, so when the close button gains focus, root.activeFocus may become false, hiding/unloading the button while it is still focused. Please verify whether root keeps activeFocus when the close button is focused; if not, consider reintroducing a focus check tied directly to the clear/close button (e.g., its activeFocus) in these conditions.
wjyrich
force-pushed
the
fix-waring
branch
4 times, most recently
from
b66d661 to
bb6de1b
Compare
18202781743
approved these changes
Mar 20, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, 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 |
更新了 closebtn的未定义问题，修复了hover通知，循环依赖问题。 更新通知焦点循环一次后，第二次循环无法经过overlapnotify，从而无法给到焦点到后面其他的通知上。 以及修复 shift+tab 保证跟tab的逻辑一致，能够正常响应焦点处理。 还需修复： 带有action的通知焦点处理。
deepin pr auto review代码审查报告本次代码变更主要集中在通知中心(NotificationCenter)的焦点管理和键盘导航逻辑上，涉及多个QML文件的修改。以下是详细的审查意见： 1. 整体评估这次提交主要优化了键盘导航体验，改进了焦点管理逻辑，移除了不必要的焦点状态标记，使代码更加简洁和可维护。整体方向是正确的，但存在一些潜在问题需要注意。 2. 语法逻辑分析2.1 正确的改进
2.2 潜在问题
3. 代码质量3.1 优点
3.2 改进建议
4. 代码性能
5. 代码安全
6. 具体建议
7. 总结这次提交改进了通知中心的焦点管理逻辑，使代码更加简洁和一致。主要关注点是确保焦点管理的一致性和边界条件的正确处理。建议在合并前进行全面的键盘导航测试，并考虑添加单元测试以验证焦点管理逻辑的正确性。 |
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.
更新了 closebtn的未定义问题，修复了hover通知，循环依赖问题。
更新通知焦点循环一次后，第二次循环无法经过overlapnotify，从而无法给到焦点到后面其他的通知上。 以及修复 shift+tab 保证跟tab的逻辑一致，能够正常响应焦点处理。
还需修复： 带有action的通知焦点处理。
Summary by Sourcery
Improve keyboard focus and close-button behavior in notification items to prevent focus loops and ensure consistent navigation.
Bug Fixes:
Enhancements: