Skip to content

Comments

fix(notification): avoid delayed-remove id collision#1452

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Ivy233:fix/bug-290767-shutdown-banner
Feb 24, 2026
Merged

fix(notification): avoid delayed-remove id collision#1452
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Ivy233:fix/bug-290767-shutdown-banner

Conversation

@Ivy233
Copy link
Contributor

@Ivy233 Ivy233 commented Feb 14, 2026

Use InvalidId (0) for delay sentinel and hover reset, preserve ids on memory replace, and update SPDX headers.

fix(notification): 修正延迟移除哨兵冲突

延迟移除使用 InvalidId(0) 避免与负数内存 ID 冲突,替换时保持内存实体 ID 一致,并更新 SPDX 版权年份。

PMS: BUG-290767

PS:修复https://github.com/linuxdeepin/dde-shell/pull/1417引入的关机提示无法消除bug。

Summary by Sourcery

Fix notification bubble delayed removal behavior to avoid ID collisions and ensure consistent entity IDs.

Bug Fixes:

  • Use a non-conflicting sentinel value for delayed-removed bubble IDs to prevent clashes with valid memory IDs.
  • Preserve the original notification entity ID when replacing it in memory to keep references consistent.

Enhancements:

  • Update SPDX copyright headers to extend coverage years across notification-related files.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 14, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Uses 0 (InvalidId) as the delayed-remove sentinel to avoid collisions with negative in-memory IDs, ensures notification entities preserve their ID on replacement, and updates SPDX copyright headers across touched files.

Sequence diagram for delayed notification removal with InvalidId sentinel

sequenceDiagram
actor User
participant BubbleQML
participant BubbleModel
participant MemoryAccessor
participant NotifyEntity

User->>BubbleQML: Move cursor over notification
BubbleQML->>BubbleModel: set delayRemovedBubble = bubble.id
activate BubbleModel
BubbleModel->>BubbleModel: m_delayRemovedBubble = bubble.id
BubbleModel->>MemoryAccessor: replaceEntity(id, entity)
activate MemoryAccessor
MemoryAccessor->>MemoryAccessor: find entity by id in m_entities
MemoryAccessor->>NotifyEntity: setId(id)
MemoryAccessor-->>BubbleModel: return id or -1
deactivate MemoryAccessor
BubbleModel-->>BubbleQML: state updated
deactivate BubbleModel

User->>BubbleQML: Move cursor away
BubbleQML->>BubbleModel: set delayRemovedBubble = 0
activate BubbleModel
BubbleModel->>BubbleModel: m_delayRemovedBubble = 0 (InvalidId sentinel)
BubbleModel-->>BubbleQML: no delayed removal pending
deactivate BubbleModel
Loading

Updated class diagram for notification bubble model and memory accessor

classDiagram

class BubbleModel {
  -int m_contentRowCount
  -int OverlayMaxCount
  -QList<qint64> m_delayBubbles
  -qint64 m_delayRemovedBubble
  -int DelayRemovBubbleTime
  +void clear()
}

class MemoryAccessor {
  -QVector<NotifyEntity> m_entities
  +qint64 replaceEntity(qint64 id, const NotifyEntity &entity)
}

class NotifyEntity {
  -qint64 m_id
  +void setId(qint64 id)
}

BubbleModel --> MemoryAccessor : uses
MemoryAccessor --> NotifyEntity : stores
Loading

File-Level Changes

Change Details Files
Use 0 as the sentinel value for delayed notification removal instead of -1 to avoid collisions, and keep C++ and QML in sync.
  • Initialize m_delayRemovedBubble to 0 instead of -1 and reset it to 0 in BubbleModel::clear
  • Update Bubble.qml to write 0 instead of -1 when clearing delayRemovedBubble on hover state changes
panels/notification/bubble/bubblemodel.cpp
panels/notification/bubble/bubblemodel.h
panels/notification/bubble/package/Bubble.qml
Ensure replaced notification memory entities retain their original ID.
  • After finding an entity by ID in MemoryAccessor::replaceEntity, assign the same ID to the replacement entity before storing it back into the list
panels/notification/common/memoryaccessor.cpp
Update SPDX copyright year ranges in modified files.
  • Change SPDX-FileCopyrightText years to a 2023 - 2026 or 2024 - 2026 range in all touched source and QML files
panels/notification/bubble/bubblemodel.cpp
panels/notification/bubble/bubblemodel.h
panels/notification/bubble/package/Bubble.qml
panels/notification/common/memoryaccessor.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.

Hey - I've left some high level feedback:

  • Consider replacing the hard-coded sentinel 0 for m_delayRemovedBubble in both C++ and QML with a shared named constant (e.g., InvalidId) to avoid magic numbers and keep the sentinel value consistent across layers.
  • When updating MemoryAccessor::replaceEntity, you now explicitly set the entity id to id; if other code paths also modify entities, it may be worth centralizing id assignment (e.g., via a helper or in NotifyEntity) to avoid future divergence in how ids are managed.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider replacing the hard-coded sentinel `0` for `m_delayRemovedBubble` in both C++ and QML with a shared named constant (e.g., `InvalidId`) to avoid magic numbers and keep the sentinel value consistent across layers.
- When updating `MemoryAccessor::replaceEntity`, you now explicitly set the entity id to `id`; if other code paths also modify entities, it may be worth centralizing id assignment (e.g., via a helper or in `NotifyEntity`) to avoid future divergence in how ids are managed.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

18202781743
18202781743 previously approved these changes Feb 24, 2026
Use InvalidId (0) for delay sentinel and hover reset, preserve ids on memory replace, and update SPDX headers.

fix(notification): 修正延迟移除哨兵冲突

延迟移除使用 InvalidId(0) 避免与负数内存 ID 冲突,替换时保持内存实体 ID 一致,并更新 SPDX 版权年份。

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

deepin pr auto review

这份代码diff主要涉及通知气泡(Bubble)模块中ID无效值的规范化处理,以及版权年份的更新。以下是详细的审查意见:

1. 语法逻辑

  • 版权年份更新:多处文件头部将版权年份从 20232024 更新为 2023 - 20262024 - 2026。这是合法的,表示对代码维护周期的预期,符合规范。
  • 引入头文件:在 bubblemodel.h 中增加了 #include "notifyentity.h"。这是必要的,因为代码中使用了 NotifyEntity::InvalidId,必须确保该头文件被包含,否则会导致编译错误。逻辑正确。
  • 常量替换:将硬编码的 -1 替换为 NotifyEntity::InvalidId
    • bubblemodel.cppbubblemodel.h 中,m_delayRemovedBubble 的初始化和赋值逻辑正确。
    • Bubble.qml 中,JavaScript/QML逻辑正确,前提是 NotifyEntity 类型或其 InvalidId 属性已正确暴露给 QML 环境。
  • 逻辑修正:在 memoryaccessor.cpp 中,replaceEntity 函数在更新实体时增加了 m_entities[index].setId(id)
    • 这是一个重要的逻辑修复。原代码在替换实体时,虽然根据 id 找到了位置并覆盖了实体,但如果传入的 entity 对象本身的 ID 与查找用的 id 不一致(例如 entity 是一个新构造的对象),容器内的对象 ID 可能会丢失或错误。显式调用 setId(id) 确保了容器内对象的 ID 始终与 Map 的 Key 保持一致。这是一个很好的改进。

2. 代码质量

  • 可读性与可维护性:使用命名常量 NotifyEntity::InvalidId 替代魔数 -1 显著提高了代码的可读性。开发者一眼就能明白该变量代表“无效ID”,而不需要去推测 -1 的含义。这也符合“Magic Numbers”代码规范。
  • 类型安全:如果 NotifyEntity::InvalidIdNotifyEntity 类的一个静态常量成员,这比裸整数 -1 更具类型安全性,有助于编译器进行类型检查(尽管 C++ 的 qint64 是基本类型,但语义上更强)。

3. 代码性能

  • 性能影响:本次修改对性能几乎没有影响。
    • 替换常量在编译期处理,运行时无开销。
    • memoryaccessor.cpp 中增加的 setId(id) 是一次简单的成员函数调用,开销极低,且对于保证数据一致性是必须的。

4. 代码安全

  • 数据一致性memoryaccessor.cpp 中的修改增强了数据一致性。确保了 m_entities 列表中的对象 ID 与索引逻辑一致,防止因 ID 错乱导致的逻辑漏洞。
  • 边界检查replaceEntity 函数使用了迭代器查找,逻辑上是安全的。Q_ASSERT 在 Debug 模式下提供了额外的保护。
  • QML 交互:在 Bubble.qml 中使用 NotifyEntity.InvalidId。这要求 C++ 端必须将 NotifyEntity 注册为 QML 类型,或者将 InvalidId 作为一个可访问的属性/常量暴露出去。如果未正确暴露,QML 将报错。建议确认 NotifyEntity 的 QML 注册情况。

改进建议

  1. QML 类型暴露确认
    请确保 NotifyEntity 类或者 InvalidId 常量已经正确暴露给 QML 引擎。通常需要在 C++ 中使用 qmlRegisterUncreatableTypeQ_PROPERTY 宏。
    例如:

    // 在 main.cpp 或相关初始化代码中
    qmlRegisterUncreatableType<NotifyEntity>("com.deepin.Notification", 1, 0, "NotifyEntity", "Cannot create NotifyEntity");

    或者如果 InvalidId 是枚举值,确保使用 Q_ENUMQ_ENUM_NS 进行了声明。

  2. setId 的必要性检查
    memoryaccessor.cpp 中,既然是根据 id 查找到的 entity,通常情况下 entity.id 应该等于 id。如果 entity 是外部传入的副本,显式 setId 是合理的。但如果 entity 是引用且来源可靠,可以考虑添加注释说明为什么需要显式设置 ID(例如:防止外部传入的对象 ID 未初始化,或者为了防御性编程)。

  3. 拼写检查
    bubblemodel.h 中,成员变量 DelayRemovBubbleTime 存在拼写错误,应为 DelayRemoveBubbleTime(Remove 而非 Remov)。虽然这次 diff 没有修改这行,但建议在后续修复,以保持代码整洁。

总结
这是一次高质量的代码重构,主要解决了“魔数”问题并修复了一个潜在的数据一致性隐患。代码逻辑清晰,符合现代 C++ 和 QML 的最佳实践。只需注意 QML 端的类型暴露即可。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, Ivy233

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

@Ivy233
Copy link
Contributor Author

Ivy233 commented Feb 24, 2026

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Feb 24, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit e1f1338 into linuxdeepin:master Feb 24, 2026
8 of 11 checks passed
@Ivy233 Ivy233 deleted the fix/bug-290767-shutdown-banner branch February 24, 2026 05:16
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