Skip to content

Conversation

@18202781743
Copy link
Contributor

@18202781743 18202781743 commented Oct 24, 2025

Fixed the hasNewNotification method to properly check both the
notification flag and the actual notification count. Previously, the
method only checked the m_hasNewNotification flag without verifying
if there were actually any notifications present. This could lead to
incorrect status display when the flag was set but no notifications
existed.

Now the method returns true only when both conditions are met: the
notification flag is set AND there is at least one notification
available. This ensures the notification indicator only appears when
there are actual notifications to display.

Influence:

  1. Test notification system with various scenarios
  2. Verify notification indicator only shows when there are actual
    notifications
  3. Test edge cases where notification count is zero but flag is set
  4. Verify normal notification flow still works correctly

fix: 修复通知状态逻辑

修复了 hasNewNotification 方法,使其正确检查通知标志和实际通知数量。之前
该方法仅检查 m_hasNewNotification 标志,而没有验证是否实际存在通知。这可
能导致在标志被设置但没有通知存在时显示错误的状态。

现在该方法仅在两个条件都满足时返回 true:通知标志被设置 AND 至少存在一个
通知。这确保通知指示器只在有实际通知可显示时出现。

Influence:

  1. 在各种场景下测试通知系统
  2. 验证通知指示器只在有实际通知时显示
  3. 测试通知数量为零但标志被设置的边界情况
  4. 验证正常通知流程仍然正常工作

Summary by Sourcery

Bug Fixes:

  • Update hasNewNotification to return true only when the notification flag is set and the count is greater than zero

Fixed the hasNewNotification method to properly check both the
notification flag and the actual notification count. Previously, the
method only checked the m_hasNewNotification flag without verifying
if there were actually any notifications present. This could lead to
incorrect status display when the flag was set but no notifications
existed.

Now the method returns true only when both conditions are met: the
notification flag is set AND there is at least one notification
available. This ensures the notification indicator only appears when
there are actual notifications to display.

Influence:
1. Test notification system with various scenarios
2. Verify notification indicator only shows when there are actual
notifications
3. Test edge cases where notification count is zero but flag is set
4. Verify normal notification flow still works correctly

fix: 修复通知状态逻辑

修复了 hasNewNotification 方法,使其正确检查通知标志和实际通知数量。之前
该方法仅检查 m_hasNewNotification 标志,而没有验证是否实际存在通知。这可
能导致在标志被设置但没有通知存在时显示错误的状态。

现在该方法仅在两个条件都满足时返回 true:通知标志被设置 AND 至少存在一个
通知。这确保通知指示器只在有实际通知可显示时出现。

Influence:
1. 在各种场景下测试通知系统
2. 验证通知指示器只在有实际通知时显示
3. 测试通知数量为零但标志被设置的边界情况
4. 验证正常通知流程仍然正常工作
@sourcery-ai
Copy link

sourcery-ai bot commented Oct 24, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

The PR updates the hasNewNotification method to combine the m_hasNewNotification flag with a positive notification count check, ensuring the indicator only appears when actual notifications exist.

Class diagram for updated Notification class logic

classDiagram
class Notification {
  - bool m_hasNewNotification
  - int m_notificationCount
  + bool hasNewNotification() const
}
Notification : hasNewNotification() returns true if m_hasNewNotification && m_notificationCount > 0
Loading

File-Level Changes

Change Details Files
Refactor hasNewNotification logic to include notification count check
  • Update return statement to 'm_hasNewNotification && m_notificationCount > 0'
plugins/dde-dock/notification/notification.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 there - I've reviewed your changes and they look great!


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

我来分析一下这个代码变更:

  1. 修改内容分析:
// 修改前
bool Notification::hasNewNotification() const
{
    return m_hasNewNotification;
}

// 修改后
bool Notification::hasNewNotification() const
{
    return m_hasNewNotification && m_notificationCount > 0;
}
  1. 改进建议:

a) 逻辑正确性:

  • 这个修改是合理的。它确保了只有在确实有新通知(m_hasNewNotification为true)且通知数量大于0时,才返回true
  • 避免了可能出现的状态不一致问题,即m_hasNewNotification为true但实际通知数量为0的情况

b) 代码质量:

  • 代码简洁清晰,易于理解
  • 建议在类中添加注释说明这两个成员变量的关系:
    // m_hasNewNotification: 表示是否有新通知的标志
    // m_notificationCount: 当前未读通知数量

c) 性能考虑:

  • 这个修改不会带来性能问题,只是增加了一个简单的条件判断
  • 两个条件判断的顺序是合理的,因为m_hasNewNotification可能是一个更轻量的检查

d) 安全性:

  • 没有安全性问题
  • 建议确保m_notificationCount是有符号整数,并添加边界检查:
    return m_hasNewNotification && (m_notificationCount > 0);
  1. 其他建议:
  • 考虑添加断言来确保这两个变量的一致性:
    Q_ASSERT(!m_hasNewNotification || m_notificationCount > 0);
  • 在resetNotificationStatus()方法中确保这两个变量同步更新

总的来说,这是一个很好的改进,提高了代码的健壮性和逻辑正确性。建议继续保持这种严谨的编码风格。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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