fix: remove redundant focus property in notification center#1250
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors focus management in the notification center by removing redundant focus flags, introducing a FocusScope for item-level focus handling, clearing focus when the center closes, and simplifying close-button focus logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Ensure the loader activation logic still covers hover scenarios if that behavior is desired, or explicitly update it to remove hover-based close button reveal.
- Fix the typo 'forcusBorderVisible' so it matches the correct 'focusBorderVisible' property name.
- Consider using an explicit focus clearing call (e.g., clearFocus()) on visibility changes instead of relying solely on
focus: root.visibleto avoid unexpected focus retention.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure the loader activation logic still covers hover scenarios if that behavior is desired, or explicitly update it to remove hover-based close button reveal.
- Fix the typo 'forcusBorderVisible' so it matches the correct 'focusBorderVisible' property name.
- Consider using an explicit focus clearing call (e.g., clearFocus()) on visibility changes instead of relying solely on `focus: root.visible` to avoid unexpected focus retention.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia 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 |
Removed explicit focus: true from OverlapNotify component as it was causing focus issues when multiple notifications are present. Added focus management to the main view container to clear focus when NotificationCenter is closed. Replaced Control with FocusScope in NotifyItemContent to better handle focus behavior and simplified the close button visibility logic. Log: Fixed focus issues in notification center Influence: 1. Test notification center opening and closing behavior 2. Verify focus transitions between notifications 3. Check close button visibility and functionality 4. Test keyboard navigation through notifications 5. Verify focus is properly cleared when center is closed fix: 修复通知中心的焦点问题 移除了OverlapNotify组件中显式的focus: true属性,该属性在多个通知同时存在 时会导致焦点问题。为主视图容器添加了焦点管理,在通知中心关闭时清除焦点。 将NotifyItemContent中的Control替换为FocusScope以更好地处理焦点行为,并简 化了关闭按钮的可见性逻辑。 Log: 修复通知中心的焦点问题 Influence: 1. 测试通知中心打开和关闭行为 2. 验证通知之间的焦点转换 3. 检查关闭按钮的可见性和功能 4. 测试通过键盘在通知间的导航 5. 验证通知中心关闭时焦点是否正确清除 PMS: BUG-284139
deepin pr auto review我对这个代码审查有以下几点意见和建议: 1. 代码逻辑方面
2. 代码质量方面
3. 代码性能方面
4. 代码安全方面
改进建议
forcusBorderVisible: visualFocus // 改为
focusBorderVisible: visualFocus
Loader {
active: {
var shouldShowCloseBtn = !(root.strongInteractive && root.actions.length > 0);
var hasFocus = root.closeVisible || activeFocus;
return shouldShowCloseBtn && hasFocus;
}
// ...
}
// FocusScope用于接收鼠标事件和管理焦点状态
FocusScope {
focus: true
// ...
}
Loader {
id: closeBtnLoader
active: {
// 使用局部变量提高可读性
var isInteractive = root.strongInteractive && root.actions.length > 0;
var shouldShowClose = root.closeVisible || activeFocus;
return !isInteractive && shouldShowClose;
}
// ...
}总的来说,这些修改主要集中在焦点管理的改进上,整体方向是正确的。但需要注意一些细节问题,如拼写错误和条件判断的完整性,以确保代码的健壮性和可维护性。 |
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Removed explicit focus: true from OverlapNotify component as it was
causing focus issues when multiple notifications are present. Added
focus management to the main view container to clear focus when
NotificationCenter is closed. Replaced Control with FocusScope in
NotifyItemContent to better handle focus behavior and simplified the
close button visibility logic.
Log: Fixed focus issues in notification center
Influence:
fix: 修复通知中心的焦点问题
移除了OverlapNotify组件中显式的focus: true属性,该属性在多个通知同时存在
时会导致焦点问题。为主视图容器添加了焦点管理,在通知中心关闭时清除焦点。
将NotifyItemContent中的Control替换为FocusScope以更好地处理焦点行为,并简
化了关闭按钮的可见性逻辑。
Log: 修复通知中心的焦点问题
Influence:
PMS: BUG-284139
Summary by Sourcery
Streamline focus management in the notification center by removing redundant focus properties, centralizing focus control, and simplifying close button logic
Bug Fixes:
Enhancements: