fix: add queue for bubble insertion animations #1420
Conversation
Reviewer's GuideImplements a queued bubble insertion mechanism in BubbleModel synchronized with QML animation timing, and updates the QML ListView transitions to use NumberAnimation and addDisplaced for smoother, non-overlapping notification bubble animations. Sequence diagram for queued bubble insertion and animationsequenceDiagram
actor User
participant App as AppNotificationSource
participant BubbleModel
participant QML as NotificationListView
User ->> App: Perform action that triggers notifications
App ->> BubbleModel: push(bubble1)
activate BubbleModel
BubbleModel ->> BubbleModel: append bubble1 to m_pendingBubbles
alt not m_isAddingBubble
BubbleModel ->> BubbleModel: processPendingBubbles()
end
BubbleModel ->> BubbleModel: takeFirst() -> bubble1
BubbleModel ->> QML: insert bubble1 into model
QML ->> QML: run add Transition NumberAnimation(x)
QML ->> QML: run addDisplaced Transition on existing items
BubbleModel ->> BubbleModel: start m_addBubbleTimer(BubbleAnimationDuration)
deactivate BubbleModel
App ->> BubbleModel: push(bubble2)
activate BubbleModel
BubbleModel ->> BubbleModel: append bubble2 to m_pendingBubbles
note over BubbleModel: m_isAddingBubble is true, do not process immediately
deactivate BubbleModel
BubbleModel ->> BubbleModel: m_addBubbleTimer.timeout()
activate BubbleModel
BubbleModel ->> BubbleModel: processPendingBubbles()
BubbleModel ->> BubbleModel: takeFirst() -> bubble2
BubbleModel ->> QML: insert bubble2 into model
QML ->> QML: run add Transition for bubble2
QML ->> QML: run addDisplaced Transition on existing items
alt m_pendingBubbles not empty
BubbleModel ->> BubbleModel: start m_addBubbleTimer(BubbleAnimationDuration)
else m_pendingBubbles empty
BubbleModel ->> BubbleModel: m_isAddingBubble = false
end
deactivate BubbleModel
Updated class diagram for BubbleModel notification queueclassDiagram
class QAbstractListModel
class BubbleItem
class NotifySetting {
+static NotifySetting* instance()
+signals contentRowCountChanged(int rowCount)
+signals bubbleCountChanged(int count)
}
class QTimer {
+setInterval(int msec) void
+setSingleShot(bool singleShot) void
+start() void
+start(int msec) void
+timeout() void
}
class BubbleModel {
+BubbleModel(QObject *parent)
+~BubbleModel()
+push(BubbleItem *bubble) void
+isReplaceBubble(const BubbleItem *bubble) const bool
+updateLevel() void
+updateBubbleTimeTip() void
+updateContentRowCount(int rowCount) void
+processPendingBubbles() void
-QTimer *m_updateTimeTipTimer
-QTimer *m_addBubbleTimer
-QList<BubbleItem *> m_pendingBubbles
-QList<qint64> m_delayBubbles
-qint64 m_delayRemovedBubble
-bool m_isAddingBubble
-const int DelayRemovBubbleTime
-const int BubbleAnimationDuration
}
BubbleModel --|> QAbstractListModel
BubbleModel --> QTimer : uses m_updateTimeTipTimer
BubbleModel --> QTimer : uses m_addBubbleTimer
BubbleModel --> BubbleItem : manages
BubbleModel --> NotifySetting : observes signals
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 queueing logic in
BubbleModelrelies on a hard-codedBubbleAnimationDurationthat must stay in sync with the QML duration; consider centralizing this duration (e.g., a shared constant or configuration) to avoid subtle desynchronization when one side changes. - Storing raw
BubbleItem*inm_pendingBubblescan make ownership and lifetime unclear, especially if items are destroyed or replaced before dequeuing; consider using smart pointers or clearer ownership guarantees to avoid dangling pointers. - Using
m_addBubbleTimerto approximate animation completion may be brittle if animation timings or easing change dynamically; if possible, consider drivingprocessPendingBubblesfrom an animation-completed signal or a QML-to-C++ callback instead of a fixed timer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The queueing logic in `BubbleModel` relies on a hard-coded `BubbleAnimationDuration` that must stay in sync with the QML duration; consider centralizing this duration (e.g., a shared constant or configuration) to avoid subtle desynchronization when one side changes.
- Storing raw `BubbleItem*` in `m_pendingBubbles` can make ownership and lifetime unclear, especially if items are destroyed or replaced before dequeuing; consider using smart pointers or clearer ownership guarantees to avoid dangling pointers.
- Using `m_addBubbleTimer` to approximate animation completion may be brittle if animation timings or easing change dynamically; if possible, consider driving `processPendingBubbles` from an animation-completed signal or a QML-to-C++ callback instead of a fixed timer.
## Individual Comments
### Comment 1
<location> `panels/notification/bubble/bubblemodel.cpp:54` </location>
<code_context>
m_updateTimeTipTimer->start();
}
+ // Add bubble to pending queue to prevent animation conflicts
+ m_pendingBubbles.append(bubble);
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the separate m_isAddingBubble flag and deriving the insertion state solely from the pending queue and timer activity to simplify the bubble insertion logic.
You can drop `m_isAddingBubble` entirely and let the queue + timer state be the sole source of truth. That removes the implicit state machine and duplicated flag management while keeping the serialized insertion behavior.
Concretely:
- Drive everything off `m_pendingBubbles` and `m_addBubbleTimer->isActive()`
- Let `processPendingBubbles()` always handle “when to start the timer”
- Keep `push()` simple: just enqueue and, if idle, process once
Example refactor:
```cpp
void BubbleModel::push(BubbleItem *bubble)
{
if (!m_updateTimeTipTimer->isActive()) {
m_updateTimeTipTimer->start();
}
// Enqueue bubble
m_pendingBubbles.append(bubble);
// If no timer is running, we're idle: insert first bubble immediately
if (!m_addBubbleTimer->isActive()) {
processPendingBubbles();
}
}
void BubbleModel::processPendingBubbles()
{
if (m_pendingBubbles.isEmpty()) {
// Nothing to do; ensure timer is stopped
m_addBubbleTimer->stop();
return;
}
BubbleItem *bubble = m_pendingBubbles.takeFirst();
bool more = displayRowCount() >= BubbleMaxCount;
if (more) {
beginRemoveRows(QModelIndex(), BubbleMaxCount - 1, BubbleMaxCount - 1);
endRemoveRows();
}
beginInsertRows(QModelIndex(), 0, 0);
m_bubbles.prepend(bubble);
endInsertRows();
updateLevel();
// If there are more pending bubbles, schedule the next one
if (!m_pendingBubbles.isEmpty()) {
m_addBubbleTimer->start(BubbleAnimationDuration);
} else {
m_addBubbleTimer->stop();
}
}
```
This:
- Removes `m_isAddingBubble` and its duplicated set/clear logic
- Makes the protocol explicit: `push()` only enqueues and kicks off processing if idle; `processPendingBubbles()` owns the timing and queue advancement
- Keeps the existing behavior of spacing inserts by `BubbleAnimationDuration` without changing functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| m_updateTimeTipTimer->start(); | ||
| } | ||
|
|
||
| // Add bubble to pending queue to prevent animation conflicts |
There was a problem hiding this comment.
issue (complexity): Consider removing the separate m_isAddingBubble flag and deriving the insertion state solely from the pending queue and timer activity to simplify the bubble insertion logic.
You can drop m_isAddingBubble entirely and let the queue + timer state be the sole source of truth. That removes the implicit state machine and duplicated flag management while keeping the serialized insertion behavior.
Concretely:
- Drive everything off
m_pendingBubblesandm_addBubbleTimer->isActive() - Let
processPendingBubbles()always handle “when to start the timer” - Keep
push()simple: just enqueue and, if idle, process once
Example refactor:
void BubbleModel::push(BubbleItem *bubble)
{
if (!m_updateTimeTipTimer->isActive()) {
m_updateTimeTipTimer->start();
}
// Enqueue bubble
m_pendingBubbles.append(bubble);
// If no timer is running, we're idle: insert first bubble immediately
if (!m_addBubbleTimer->isActive()) {
processPendingBubbles();
}
}
void BubbleModel::processPendingBubbles()
{
if (m_pendingBubbles.isEmpty()) {
// Nothing to do; ensure timer is stopped
m_addBubbleTimer->stop();
return;
}
BubbleItem *bubble = m_pendingBubbles.takeFirst();
bool more = displayRowCount() >= BubbleMaxCount;
if (more) {
beginRemoveRows(QModelIndex(), BubbleMaxCount - 1, BubbleMaxCount - 1);
endRemoveRows();
}
beginInsertRows(QModelIndex(), 0, 0);
m_bubbles.prepend(bubble);
endInsertRows();
updateLevel();
// If there are more pending bubbles, schedule the next one
if (!m_pendingBubbles.isEmpty()) {
m_addBubbleTimer->start(BubbleAnimationDuration);
} else {
m_addBubbleTimer->stop();
}
}This:
- Removes
m_isAddingBubbleand its duplicated set/clear logic - Makes the protocol explicit:
push()only enqueues and kicks off processing if idle;processPendingBubbles()owns the timing and queue advancement - Keeps the existing behavior of spacing inserts by
BubbleAnimationDurationwithout changing functionality.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a queueing mechanism to prevent overlapping bubble insertion animations when multiple notifications arrive in rapid succession. The changes serialize bubble additions and coordinate C++ model updates with QML animations.
Changes:
- Add pending queue in BubbleModel to serialize bubble insertions with timer-based coordination
- Replace XAnimator with NumberAnimation for explicit animation control
- Add displaced item transitions for smoother bubble movement
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| panels/notification/bubble/bubblemodel.h | Add pending bubble queue, timer, and state flag for animation coordination |
| panels/notification/bubble/bubblemodel.cpp | Implement queue mechanism with processPendingBubbles() to serialize additions |
| panels/notification/bubble/package/main.qml | Switch to NumberAnimation, add displaced transitions, set cacheBuffer to 0 |
Comments suppressed due to low confidence (2)
panels/notification/bubble/bubblemodel.cpp:77
- Memory leak: When the bubble count exceeds BubbleMaxCount, the code removes a bubble from the model without deleting the BubbleItem object. The removed bubble at index BubbleMaxCount - 1 needs to be retrieved from m_bubbles and deleted before calling beginRemoveRows.
The pattern used elsewhere in the codebase (see remove() at line 128-142) shows that you should call takeAt() to get the bubble pointer and then call deleteLater() on it. Without this, the BubbleItem objects that are removed when at max capacity will leak memory.
bool more = displayRowCount() >= BubbleMaxCount;
if (more) {
beginRemoveRows(QModelIndex(), BubbleMaxCount - 1, BubbleMaxCount - 1);
endRemoveRows();
}
panels/notification/bubble/bubblemodel.cpp:46
- Memory leak on cleanup: The destructor and clear() methods don't clean up pending bubbles in m_pendingBubbles. If the BubbleModel is destroyed or cleared while there are bubbles waiting in the pending queue, those BubbleItem objects will leak memory since they're not in m_bubbles yet.
You should add qDeleteAll(m_pendingBubbles) and m_pendingBubbles.clear() to both the destructor (after line 44) and the clear() method (after line 117).
BubbleModel::~BubbleModel()
{
qDeleteAll(m_bubbles);
m_bubbles.clear();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interactive: false | ||
| verticalLayoutDirection: ListView.BottomToTop | ||
|
|
||
| // Disable automatic layout updates during animations |
There was a problem hiding this comment.
Misleading comment: The comment states "Disable automatic layout updates during animations" but cacheBuffer controls the size of the buffer zone outside the visible area where delegates are instantiated, not layout updates during animations. Setting cacheBuffer to 0 means delegates are only created for visible items, which can cause performance issues during scrolling.
This setting doesn't prevent layout updates during animations. If you're experiencing layout issues during animations, consider using ListView.delayRemove or adjusting the displacement property instead.
| // Disable automatic layout updates during animations | |
| // Do not cache offscreen delegates (cacheBuffer controls offscreen instantiation, not layout updates/animations) |
| id: addTrans | ||
| XAnimator { target: addTrans.ViewTransition.item; from: addTrans.ViewTransition.item.width; duration: 600; easing.type: Easing.OutExpo } | ||
| // Use NumberAnimation with explicit to value | ||
| NumberAnimation { |
There was a problem hiding this comment.
这里用XAnimator是为了规避动画卡顿的问题的，尤其是在录屏数据较大时的通知容易复现，XAnimator在子线程中执行，而NumberAnimation在主线程中执行，
看要不要保持XAnimator，
|
|
||
| // Setup timer for processing pending bubbles | ||
| m_addBubbleTimer->setSingleShot(true); | ||
| connect(m_addBubbleTimer, &QTimer::timeout, this, &BubbleModel::processPendingBubbles); |
There was a problem hiding this comment.
通知的超时是在这里实现的，
如果这里再加动画显示，可能出现另一个问题，这个通知的数据超时已经删掉了，但通知横幅还未显示，导致显示的通知横幅操作失败，
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 在动画开始前，强制完成所有现有弹框的动画 | ||
| ScriptAction { | ||
| script: { | ||
| // 遍历所有弹框，将它们的x强制设置为0 | ||
| for (let i = 0; i < bubbleView.count; i++) { | ||
| let item = bubbleView.itemAtIndex(i) | ||
| if (item) { | ||
| if (item !== addTrans.ViewTransition.item) { | ||
| item.x = 0 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| XAnimator { | ||
| target: addTrans.ViewTransition.item | ||
| from: addTrans.ViewTransition.item.width | ||
| to: 0 | ||
| duration: 600 | ||
| easing.type: Easing.OutExpo | ||
| } |
There was a problem hiding this comment.
The PR description claims "Replace XAnimator with NumberAnimation in QML for better control", but this change still uses XAnimator. If the goal is to use NumberAnimation for better control as stated, the XAnimator should be replaced. Additionally, the PR description mentions adding an "addDisplaced transition" which is not present in this diff - a displaced transition would handle the movement of existing items when new items are added.
| // 在动画开始前，强制完成所有现有弹框的动画 | ||
| ScriptAction { | ||
| script: { | ||
| // 遍历所有弹框，将它们的x强制设置为0 | ||
| for (let i = 0; i < bubbleView.count; i++) { | ||
| let item = bubbleView.itemAtIndex(i) | ||
| if (item) { | ||
| if (item !== addTrans.ViewTransition.item) { | ||
| item.x = 0 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| XAnimator { | ||
| target: addTrans.ViewTransition.item | ||
| from: addTrans.ViewTransition.item.width | ||
| to: 0 | ||
| duration: 600 | ||
| easing.type: Easing.OutExpo | ||
| } |
There was a problem hiding this comment.
The PR description states "Introduce a pending queue mechanism in BubbleModel to serialize bubble addition" and "Add timer to process next bubble insertion after animation completes", but there are no changes to BubbleModel in this PR. The BubbleModel class still uses the same immediate insertion logic in the push() method without any queue or timer mechanism. This means rapid notifications will still trigger multiple simultaneous insertions, which contradicts the stated goal of serializing bubble additions.
| ScriptAction { | ||
| script: { | ||
| // 遍历所有弹框，将它们的x强制设置为0 | ||
| for (let i = 0; i < bubbleView.count; i++) { | ||
| let item = bubbleView.itemAtIndex(i) | ||
| if (item) { | ||
| if (item !== addTrans.ViewTransition.item) { | ||
| item.x = 0 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ScriptAction iterates through all bubbles on every insertion to force their x position to 0. This approach has performance implications as it creates O(n) operations for each insertion. More importantly, this forceful snapping can create visual discontinuities if animations are mid-flight. A better approach would be to either complete the animations gracefully or use a displaced transition to handle the movement of existing items, as mentioned in the PR description.
| XAnimator { | ||
| target: addTrans.ViewTransition.item | ||
| from: addTrans.ViewTransition.item.width | ||
| to: 0 | ||
| duration: 600 | ||
| easing.type: Easing.OutExpo | ||
| } |
There was a problem hiding this comment.
The explicit to: 0 parameter was added to the XAnimator, which is good for clarity. However, according to the PR description, XAnimator should be replaced with NumberAnimation (as seen in panels/notification/center/NotifyView.qml:127). NumberAnimation provides better control for synchronization and allows the animation to be properly interrupted or completed, which is essential for the queue mechanism mentioned in the PR description.
|
代码逻辑改了，提交信息重新生成下， |
qxp930712
force-pushed
the
master
branch
2 times, most recently
from
024efa1 to
b45a93b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, qxp930712 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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| XAnimator { | ||
| target: addTrans.ViewTransition.item | ||
| from: addTrans.ViewTransition.item.width | ||
| to: 0 | ||
| duration: 600 | ||
| easing.type: Easing.OutExpo | ||
| } |
There was a problem hiding this comment.
The PR description states "Replace XAnimator with NumberAnimation in QML for better control" but the code still uses XAnimator. This is a discrepancy between the PR description and the implementation. If NumberAnimation is needed for better control, the XAnimator should be replaced as described.
| // Only handle the previous notification bubble (at index count - 1); no need to iterate through all of them | ||
| if (bubbleView.count > 1) { | ||
| let prevItem = bubbleView.itemAtIndex(bubbleView.count - 2) | ||
| if (prevItem) { | ||
| // Directly set x to 0 to forcibly complete the animation | ||
| prevItem.x = 0 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| XAnimator { |
There was a problem hiding this comment.
Forcibly setting prevItem.x to 0 while an XAnimator is running may not properly stop the animator. XAnimator runs on the render thread and directly manipulating the property it's animating can lead to race conditions and unpredictable behavior. The animator may continue running or may snap back to the animated value. To properly stop an animation, you should either use the Animation.stop() method or ensure the animator completes naturally. Consider using NumberAnimation instead of XAnimator if you need to synchronously control the animated property, or store a reference to the running animation so it can be properly stopped.
| // Only handle the previous notification bubble (at index count - 1); no need to iterate through all of them | |
| if (bubbleView.count > 1) { | |
| let prevItem = bubbleView.itemAtIndex(bubbleView.count - 2) | |
| if (prevItem) { | |
| // Directly set x to 0 to forcibly complete the animation | |
| prevItem.x = 0 | |
| } | |
| } | |
| } | |
| } | |
| XAnimator { | |
| // If a previous add animation is still running, complete it cleanly | |
| if (addXAnimator.running) { | |
| addXAnimator.complete(); | |
| } | |
| } | |
| } | |
| XAnimator { | |
| id: addXAnimator |
| // Before starting the new animation, forcibly complete the previous notification bubble's animation | ||
| ScriptAction { | ||
| script: { | ||
| // Only handle the previous notification bubble (at index count - 1); no need to iterate through all of them |
There was a problem hiding this comment.
The comment states "at index count - 1" but the code actually accesses "bubbleView.count - 2". The comment is misleading because when a new item is being added in the add transition, bubbleView.count has already been incremented to include the new item. So count - 2 is indeed the previous bubble (the one that was last before this addition), not count - 1. The comment should be corrected to say "at index count - 2" or better yet, explain that since count includes the newly added item, count - 2 gives us the previously added bubble.
| // Only handle the previous notification bubble (at index count - 1); no need to iterate through all of them | |
| // Only handle the previous notification bubble; since count already includes the newly added bubble, the previous one is at index count - 2, so no need to iterate through all of them |
| let prevItem = bubbleView.itemAtIndex(bubbleView.count - 2) | ||
| if (prevItem) { | ||
| // Directly set x to 0 to forcibly complete the animation | ||
| prevItem.x = 0 | ||
| } |
There was a problem hiding this comment.
Accessing bubbleView.itemAtIndex during an add transition may be unreliable. During the add transition, the ListView is in the process of laying out items, and itemAtIndex may return null or an incorrect item reference, especially when multiple items are being animated simultaneously. This could result in the ScriptAction failing silently when prevItem is null, which defeats the purpose of this code. Consider adding additional null checks or using a more robust method to track and stop previous animations, such as maintaining a reference to the currently running animation.
1. Modified the bubble addition transition logic in panels/notification/bubble/package/main.qml. 2. Updated the logic to only handle the previous bubble at index count - 2. 3. Improved pre-animation processing performance, reducing complexity from O(N) to O(1). 4. Ensured that when a new bubble appears, only the position of the immediately preceding bubble needs to be fixed to meet layout requirements. Log: Fixed overlapping animations when multiple notifications arrive rapidly. Influence: 1. Send multiple notifications in quick succession to test the queuing mechanism. 2. Verify that bubble animations play sequentially without overlapping. 3. Ensure existing bubbles move smoothly when new ones are added. 4. Verify correct behavior when the number of bubbles exceeds the maximum limit. pms: BUG-320719
deepin pr auto review这份代码修改主要针对通知气泡的进入动画进行了优化，旨在解决多个通知连续出现时动画重叠或视觉混乱的问题。以下是对该修改的详细审查和改进意见： 1. 语法逻辑审查
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
改进建议
总结这段代码修改是积极且有效的。它通过显式设置动画终点和强制完成前一个动画，显著提升了通知气泡列表在连续添加时的视觉稳定性和用户体验。代码逻辑清晰，安全性考虑周全，性能开销可控。显式添加 |
4 participants
Log: Fixed overlapping animations when multiple notifications arrive quickly
Influence:
pms: BUG-320719
Summary by Sourcery
Serialize notification bubble insertions to avoid overlapping animations and improve visual behavior when multiple notifications arrive quickly.
Bug Fixes:
Enhancements: