Skip to content

feat: add bubble removal animation#1461

Open
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
qxp930712:master
Open

feat: add bubble removal animation#1461
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
qxp930712:master

Conversation

@qxp930712
Copy link

  1. Implement asynchronous removal logic in BubbleModel to support QML animations
  2. Add bubbleAboutToRemove signal and removeAnimationDuration property
  3. Delay actual data removal until animation completes using QTimer::singleShot
  4. Introduce slide-out and fade-out transitions in Bubble.qml and main.qml
  5. Set ListView cacheBuffer to 0 to ensure proper transition rendering
  6. Add removeDisplaced transition for smooth repositioning of remaining items

Log: Notification bubbles will now slide out smoothly when closed instead of disappearing instantly.

Influence:

  1. Close a notification bubble and observe the slide-out and fade animation
  2. Verify the bubble is only removed from the model after the animation finishes
  3. Test removing multiple bubbles rapidly to check for visual artifacts
  4. Verify that remaining bubbles slide up smoothly when a bubble is removed
  5. Check model consistency after multiple remove operations
  6. Confirm that hovering over a bubble does not interrupt the removal animation

PMS: BUG-284659

1. Implement asynchronous removal logic in BubbleModel to support QML
animations
2. Add bubbleAboutToRemove signal and removeAnimationDuration property
3. Delay actual data removal until animation completes using
QTimer::singleShot
4. Introduce slide-out and fade-out transitions in Bubble.qml and
main.qml
5. Set ListView cacheBuffer to 0 to ensure proper transition rendering
6. Add removeDisplaced transition for smooth repositioning of remaining
items

Log: Notification bubbles will now slide out smoothly when closed
instead of disappearing instantly.

Influence:
1. Close a notification bubble and observe the slide-out and fade
animation
2. Verify the bubble is only removed from the model after the animation
finishes
3. Test removing multiple bubbles rapidly to check for visual artifacts
4. Verify that remaining bubbles slide up smoothly when a bubble is
removed
5. Check model consistency after multiple remove operations
6. Confirm that hovering over a bubble does not interrupt the removal
animation

PMS: BUG-284659
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qxp930712

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @qxp930712, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link

deepin pr auto review

Git Diff 代码审查

总体评价

这段代码实现了通知气泡的移除动画功能,通过延迟实际删除操作来确保动画能够完整播放。整体逻辑基本合理,但存在一些潜在的问题和可以改进的地方。

详细审查

1. 语法逻辑

问题1:BubbleModel::removeById 中的双重循环

// 在 removeById 中有双重循环查找相同ID的项目
for (const auto &item : m_bubbles) {
    if (item->id() == id) {
        // ...
        QTimer::singleShot(m_removeAnimationDuration, this, [this, id]() {
            for (const auto &item : m_bubbles) {  // 重复的循环
                if (item->id() == id) {
                    remove(m_bubbles.indexOf(item));
                    break;
                }
            }
        });
        return item;
    }
}

改进建议:可以在第一次循环中保存找到的项目指针,然后在定时器回调中使用该指针,避免重复循环:

BubbleItem *targetItem = nullptr;
for (const auto &item : m_bubbles) {
    if (item->id() == id) {
        targetItem = item;
        m_delayBubbles.removeAll(id);
        Q_EMIT bubbleAboutToRemove(id);
        break;
    }
}

if (targetItem) {
    QTimer::singleShot(m_removeAnimationDuration, this, [this, targetItem]() {
        if (m_bubbles.contains(targetItem)) {
            remove(m_bubbles.indexOf(targetItem));
        }
    });
    return targetItem;
}

问题2:setRemoveAnimationDuration 中的硬编码偏移量

void BubbleModel::setRemoveAnimationDuration(int duration)
{
    m_removeAnimationDuration = duration + 100;  // 硬编码的100ms偏移
    Q_EMIT removeAnimationDurationChanged();
}

改进建议:应该将这个偏移量定义为常量,并添加注释说明其用途:

// 在类中定义常量
static constexpr int RemoveAnimationBuffer = 100; // 缓冲时间,确保动画完成后再删除

void BubbleModel::setRemoveAnimationDuration(int duration)
{
    m_removeAnimationDuration = duration + RemoveAnimationBuffer;
    Q_EMIT removeAnimationDurationChanged();
}

2. 代码质量

问题1:缺少对空指针的检查

BubbleModel::removeById 中,没有检查 item 是否为空指针,虽然不太可能发生,但为了健壮性应该添加检查。

问题2:QML中的状态管理

Bubble.qml 中,isRemoving 属性在气泡被移除后不会被重置,如果对象被重用可能会导致问题。

改进建议:在气泡被移除后重置状态:

Connections {
    target: Applet.bubbles
    function onBubbleAboutToRemove(id) {
        if (id === bubble.id) {
            control.isRemoving = true
        }
    }
    function onRowsRemoved() {
        // 当行被移除时重置状态
        control.isRemoving = false
    }
}

3. 代码性能

问题1:ListView 的 cacheBuffer 设置

ListView {
    // ...
    cacheBuffer: 0
    // ...
}

改进建议:将 cacheBuffer 设置为 0 可能会影响性能,特别是在滚动时。应该根据气泡的高度和预期显示数量设置一个合理的值:

ListView {
    // ...
    cacheBuffer: 360 * 2  // 缓存两个气泡的高度
    // ...
}

问题2:频繁的信号连接

Bubble.qml 中,每个气泡都连接到 Applet.bubbles 的信号,如果有大量气泡,这可能会导致性能问题。

改进建议:考虑使用 ListViewremove 信号来处理移除动画,而不是在每个气泡中单独连接信号。

4. 代码安全

问题1:潜在的内存泄漏

BubbleModel::removeById 中,如果定时器回调中的 remove 操作失败(例如,项目已经被移除),可能会导致内存泄漏。

改进建议:在定时器回调中添加检查,确保项目仍然存在:

QTimer::singleShot(m_removeAnimationDuration, this, [this, id]() {
    for (const auto &item : m_bubbles) {
        if (item->id() == id) {
            remove(m_bubbles.indexOf(item));
            break;
        }
    }
    // 如果没有找到项目,可能需要额外的清理逻辑
});

问题2:竞态条件

BubbleModel::removeById 中,如果在定时器触发前再次调用 removeById,可能会导致不一致的状态。

改进建议:添加一个集合来跟踪正在被移除的气泡,防止重复操作:

// 在类中添加成员变量
QSet<qint64> m_removingBubbles;

BubbleItem *BubbleModel::removeById(qint64 id)
{
    if (m_removingBubbles.contains(id)) {
        return nullptr;
    }
    
    for (const auto &item : m_bubbles) {
        if (item->id() == id) {
            m_removingBubbles.insert(id);
            m_delayBubbles.removeAll(id);
            Q_EMIT bubbleAboutToRemove(id);
            QTimer::singleShot(m_removeAnimationDuration, this, [this, id]() {
                m_removingBubbles.remove(id);
                for (const auto &item : m_bubbles) {
                    if (item->id() == id) {
                        remove(m_bubbles.indexOf(item));
                        break;
                    }
                }
            });
            return item;
        }
    }
    return nullptr;
}

总结

这段代码实现了通知气泡的移除动画功能,但存在一些潜在的问题,包括重复循环、硬编码值、状态管理、性能问题和潜在的内存泄漏。通过上述改进建议,可以提高代码的健壮性、性能和可维护性。

特别是需要关注以下几点:

  1. 避免重复循环,提高效率
  2. 使用常量代替硬编码值
  3. 改进状态管理,确保状态正确重置
  4. 合理设置 ListView 的缓存
  5. 添加安全检查,防止竞态条件和内存泄漏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants