Skip to content

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

Open
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233:fix/bug-290767-shutdown-banner
Open

fix(notification): avoid delayed-remove id collision#1452
Ivy233 wants to merge 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.

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 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

@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.

@deepin-ci-robot
Copy link

deepin pr auto review

这份代码变更主要涉及通知气泡模型、内存访问器以及相关QML文件的修改。以下是对代码的详细审查意见,分为几个方面:

1. 语法逻辑

  • 版权年份更新

    • bubblemodel.cpp, bubblemodel.h, Bubble.qml, memoryaccessor.cpp
    • 意见:将版权年份从 20232024 更新为 2023 - 20262024 - 2026 符合规范,表明项目维护的延续性。这部分修改没有问题。
  • m_delayRemovedBubble 初始值变更

    • bubblemodel.cpp, bubblemodel.h, Bubble.qml
    • 变更内容:将 m_delayRemovedBubble 的默认值/初始值从 -1 修改为 0
    • 逻辑审查
      • Bubble.qml 中,逻辑是:当鼠标悬停时,delayRemovedBubble 设为当前气泡的 ID;当鼠标移出时,设为 0(原为 -1)。
      • bubblemodel.cppclear() 函数中,重置时也将其设为 0
      • 潜在风险:通常在数据库或ID系统中,0 也是一个有效的 ID(例如自增主键从 1 开始,或者某些系统用 0 表示特殊实体)。如果通知系统的气泡 ID(bubble.id)可能为 0,那么当鼠标移出设为 0 时,逻辑上会误判为正在悬停 ID 为 0 的气泡,从而导致该气泡无法被自动移除(如果 0 是有效 ID)。如果 ID 严格为正整数(>0),使用 0 作为"无效"或"无选中"状态是可以接受的。
      • 建议:请确认 NotifyEntity 或气泡 ID 的生成规则。如果 ID 可能包含 0,建议保留 -1 或使用 std::numeric_limits<qint64>::min() 等明显不可能的值作为无效状态。
  • MemoryAccessor::replaceEntity 逻辑补充

    • memoryaccessor.cpp
    • 变更内容:在替换实体时,显式调用 m_entities[index].setId(id)
    • 逻辑审查
      • 代码先执行了 m_entities[index] = entity(赋值操作),随后调用了 setId(id)
      • 这意味着 entity 参数传入时的 ID 可能与查找用的 id 参数不一致,或者赋值操作没有正确同步 ID(例如 NotifyEntity 的拷贝赋值运算符实现有缺陷)。
      • 改进意见:这个修改是为了修复 Bug(ID 未正确更新)。从逻辑上看,先赋值再强制设置 ID 是安全的,确保了容器中的对象 ID 与查找键一致。

2. 代码质量

  • 命名规范

    • DelayRemovBubbleTime (在 bubblemodel.h 中)
    • 意见:存在拼写错误,应为 DelayRemoveBubbleTime(Remove 拼写错误)。虽然不影响编译,但建议修正以保持代码专业性。
  • 类型一致性

    • m_delayRemovedBubble 使用了 qint64
    • 意见:Qt 的 model 索引或 ID 通常使用 intQString。如果 bubble.idqint64,那么这里类型匹配是正确的。请确保 QML 中的 bubble.id 和 C++ 中的类型一致,避免隐式类型转换带来的精度丢失或警告。

3. 代码性能

  • 性能影响
    • m_delayRemovedBubble 的赋值操作非常轻量,无论是设为 -1 还是 0,对性能无影响。
    • MemoryAccessor 中的修改增加了函数调用,但在替换操作中这是必要的开销,对性能影响可忽略不计。

4. 代码安全

  • ID 的有效性检查

    • 关于将 m_delayRemovedBubble 改为 0
    • 安全建议:如果 bubble.id 是由外部输入或不可控来源生成的,必须确保 0 永远不会被分配给一个真实的气泡。否则,攻击者或异常情况可能导致 ID 为 0 的气泡在鼠标移出其他气泡时,意外地被判定为"悬停"状态,从而产生逻辑漏洞(如不消失)。
    • 防御性编程:建议在 BubbleModel 中添加断言或注释,明确声明 ID 必须大于 0:
      // 确保 ID 始终为正数,0 被保留用于表示"无选中"状态
      Q_ASSERT(entity.id() > 0);
  • 内存访问器

    • MemoryAccessor::replaceEntity 中使用了 Q_ASSERT(index >= 0)。这是调试时的有效检查,但在 Release 版本中 Q_ASSERT 可能会被移除。
    • 建议:如果 index 计算错误(虽然 iter 是基于 id 查找到的,理论上没问题),直接访问 m_entities[index] 可能越界。不过由于逻辑是先 find 再计算偏移,安全性较高。

总结与改进建议

  1. 关于 ID 值 0 的使用

    • 最关键的建议:请务必核实 NotifyEntity 的 ID 是否可能为 0
    • 如果 ID 是数据库自增主键(通常从 1 开始),改为 0 是安全的,且比 -1 更符合无符号类型的习惯(如果未来改为 quint64)。
    • 如果 ID 可能是 0请回退此修改,或者定义一个常量来表示无效 ID,例如:
      static const qint64 INVALID_BUBBLE_ID = -1; // 或者 0,取决于业务逻辑
      并在代码中使用该常量,而不是硬编码数字。
  2. 修复拼写错误

    • DelayRemovBubbleTime 修正为 DelayRemoveBubbleTime
  3. 代码注释

    • bubblemodel.h 中为 m_delayRemovedBubble 添加注释,说明 0 代表"无延迟移除的气泡",提高代码可读性。
  4. MemoryAccessor 的修改

    • 目前的修改 m_entities[index].setId(id) 是合理的,确保了数据一致性。无需进一步修改。

综合评分
代码修改逻辑基本清晰,主要风险集中在 m_delayRemovedBubble 的取值上。只要确保业务层 ID 不会为 0,该变更是可以接受的。同时建议修复头文件中的拼写错误。

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.

2 participants