Skip to content

fix: add queue for bubble insertion animations #1420

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.

Sign up for GitHub

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

Merged
18202781743 merged 1 commit into from
Feb 2, 2026
Merged

fix: add queue for bubble insertion animations #1420

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion panels/notification/bubble/package/main.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,26 @@ Window {
verticalLayoutDirection: ListView.BottomToTop
add: Transition {
id: addTrans
XAnimator { target: addTrans.ViewTransition.item; from: addTrans.ViewTransition.item.width; duration: 600; easing.type: Easing.OutExpo }
// 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
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
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
}
Comment on lines +78 to +82
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}
}
XAnimator {
Comment on lines +76 to +86
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
target: addTrans.ViewTransition.item
from: addTrans.ViewTransition.item.width
to: 0
duration: 600
easing.type: Easing.OutExpo
}
Comment on lines +86 to +92
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +92
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
delegate: Bubble {
width: 360
Expand Down