-
Notifications
You must be signed in to change notification settings - Fork 55
feat: enhance notification animations and visual effects #1337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideEnhances notification panel animations by replacing legacy state-based transitions with ListView-based transitions gated by panel visibility, introduces indexInGroup and panelShown properties for grouping and visibility control, improves overlap indicator effects, corrects layout calculations and adds z-index management for proper layering. Sequence diagram for notification add/remove animations with ListView transitionssequenceDiagram
participant User
participant NotificationPanel
participant ListView
participant NotificationItem
User->>NotificationPanel: Open panel
NotificationPanel->>ListView: Set panelShown = true
ListView->>NotificationItem: Add notification (with indexInGroup)
ListView->>NotificationItem: Animate add (if panelShown)
User->>NotificationPanel: Remove notification
NotificationPanel->>ListView: Remove notification
ListView->>NotificationItem: Animate remove (if panelShown)
Entity relationship diagram for notification item grouping and index trackingerDiagram
NOTIFY_MODEL ||--o{ APP_NOTIFY_ITEM : contains
APP_NOTIFY_ITEM {
int indexInGroup
bool pinned
bool strongInteractive
}
NOTIFY_MODEL {
int NotifyIndexInGroup
}
Class diagram for updated notification item and model structureclassDiagram
class NotifyModel {
+open()
+close()
+expandApp(row)
+data(index, role)
+roleNames()
NotifyIndexInGroup
}
class AppNotifyItem {
+int indexInGroup
+void setIndexInGroup(int)
+int indexInGroup() const
+bool pinned()
+bool strongInteractive()
}
NotifyModel "1" -- "*" AppNotifyItem : contains
Class diagram for QML notification item properties and transitionsclassDiagram
class NotifyItem {
+int indexInGroup
+signal remove()
+signal dismiss()
+signal actionInvoked(actionId)
}
class NotifyView {
+bool panelShown
+Transition add
+Transition remove
+Transition addDisplaced
+Transition removeDisplaced
}
class OverlapIndicator {
+bool enableAnimation
}
NotifyView "1" -- "*" NotifyItem : displays
NotifyItem "1" -- "1" OverlapIndicator : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Relying on DS.singleShot with a fixed 100 ms delay to set panelShown may be brittle; consider using QML state changes or built-in visibility signals to toggle animations in sync with the panel lifecycle.
- There are a lot of hard-coded magic values (animation durations, offsets like 24px, z-indices, and the manual text width calculation); extracting these into named constants or helper bindings will improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on DS.singleShot with a fixed 100 ms delay to set panelShown may be brittle; consider using QML state changes or built-in visibility signals to toggle animations in sync with the panel lifecycle.
- There are a lot of hard-coded magic values (animation durations, offsets like 24px, z-indices, and the manual text width calculation); extracting these into named constants or helper bindings will improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `panels/notification/plugin/NotifyItemContent.qml:212-216` </location>
<code_context>
Layout.alignment: Qt.AlignLeft | Qt.AlignTop
- Layout.fillWidth: true
+ // text 宽度若让Layout通过implicitWidth计算会导致ListView的add动画出现位置错误,故这里手动计算Text的宽度
+ Layout.preferredWidth: NotifyStyle.contentItem.width - appIcon.width
+ - appIcon.Layout.leftMargin - appIcon.Layout.rightMargin
+ - contentLayout.Layout.rightMargin - contentLayout.Layout.leftMargin
+ - (contentIconLoader.active ? (contentIconLoader.width + 1) : 0)
+ - bodyRow.spacing * bodyRow.children.length - 1
visible: text !== ""
text: root.content
</code_context>
<issue_to_address>
**issue:** Manual width calculation for bodyText may be fragile if layout changes.
This calculation depends on several component properties, making it prone to errors if any layout details change. To mitigate future issues, either document these dependencies clearly or encapsulate the logic in a dedicated function.
</issue_to_address>
### Comment 2
<location> `panels/notification/plugin/OverlapIndicator.qml:44-46` </location>
<code_context>
topMargin: revert ? undefined : -(height - overlapHeight)
bottomMargin: revert ? -(height - overlapHeight) : undefined
}
+ opacity: 0
+
+ Component.onCompleted: {
+ if (root.enableAnimation) {
+ fadeInAnimation.start()
</code_context>
<issue_to_address>
**issue:** Animation logic for opacity may not handle repeated show/hide cycles correctly.
Currently, fade-in only occurs on initial completion. To support repeated show/hide cycles and changes to enableAnimation, consider updating the logic to respond to property changes or use a state-driven approach.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
TAG Bot New tag: 2.0.18 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1. Removed custom state-based removal animations from NormalNotify and OverlapNotify components 2. Added sophisticated ListView transitions for add/remove operations with smooth animations 3. Implemented indexInGroup property to track notification position within groups 4. Added panelShown property to control animation activation only when panel is visible 5. Enhanced OverlapIndicator with staggered fade-in animations for better visual experience 6. Fixed text width calculation in NotifyItemContent to prevent layout issues during animations 7. Added proper z-index management for notification items to ensure correct layering Log: Improved notification panel animations with smoother transitions and better visual effects Influence: 1. Test notification panel opening and closing animations 2. Verify smooth add/remove transitions for individual notifications 3. Check group notification animations and overlap indicator effects 4. Test notification content display with various text lengths and icons 5. Verify animation performance with multiple notifications 6. Test notification interactions (remove, dismiss, action invoke) during animations feat: 增强通知动画和视觉效果 1. 从 NormalNotify 和 OverlapNotify 组件中移除基于自定义状态的删除动画 2. 为添加/删除操作添加复杂的 ListView 过渡动画,实现平滑的动画效果 3. 实现 indexInGroup 属性来跟踪通知在组内的位置 4. 添加 panelShown 属性控制动画仅在面板可见时激活 5. 增强 OverlapIndicator,添加错开的淡入动画以获得更好的视觉体验 6. 修复 NotifyItemContent 中的文本宽度计算,防止动画期间的布局问题 7. 添加适当的 z-index 管理确保通知项的正确层级 Log: 改进了通知面板动画,提供更平滑的过渡和更好的视觉效果 Influence: 1. 测试通知面板打开和关闭动画 2. 验证单个通知的平滑添加/删除过渡效果 3. 检查组通知动画和重叠指示器效果 4. 测试不同文本长度和图标的通知内容显示 5. 验证多通知情况下的动画性能 6. 测试动画期间的通知交互(删除、关闭、操作调用) pms: BUG-338883
|
/forcemerge |
|
This pr force merged! (status: blocked) |
deepin pr auto review我来对这个diff进行审查:
总体来说,这次改动主要优化了通知中心的动画效果和代码结构,改动是积极的。代码质量良好,逻辑清晰,没有明显的性能或安全问题。建议在后续版本中持续观察动画效果的实际表现,并根据用户反馈进行微调。 |
Log: Improved notification panel animations with smoother transitions and better visual effects
Influence:
feat: 增强通知动画和视觉效果
Log: 改进了通知面板动画,提供更平滑的过渡和更好的视觉效果
Influence:
pms: BUG-338883# 请为您的变更输入提交说明。以 '#' 开始的行将被忽略,而一个空的提交
Summary by Sourcery
Enhance the notification panel with smooth, configurable animations for adding and removing items, introduce indexInGroup and panelShown properties to control behavior, improve overlap indicator effects and layering, and fix content layout calculation
New Features:
Bug Fixes:
Enhancements:
Chores: