Skip to content

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented Sep 24, 2025

Added a "Clean All" button to overlap notification items to allow users to remove all notifications from a specific app at once The button is implemented as a custom button component in OverlapNotify with proper translation support
Modified NotifyModel to handle removal of overlap type notifications and added warning logging for missing apps
Updated translation files across multiple languages to include the new "Clean All" text

feat: 为重叠通知添加清除全部按钮

在重叠通知项中添加"清除全部"按钮,允许用户一次性移除特定应用的所有通知
该按钮作为自定义按钮组件在 OverlapNotify 中实现,并支持多语言翻译
修改 NotifyModel 以处理重叠类型通知的移除,并为不存在的应用添加警告日志
更新了多个语言的翻译文件以包含新的"清除全部"文本

Summary by Sourcery

Add a “Clean All” button to overlap notification items for batch dismissal by app; integrate a custom button component in QML, extend model logic to remove overlap-type notifications with warning logs for missing apps, and update translations to include the new label

New Features:

  • Add a “Clean All” button to overlap notifications to clear all notifications for a specific app at once

Enhancements:

  • Introduce customButtonComponent support in NotifyItemContent and wire it through OverlapNotify and NotifyViewDelegate for flexible action buttons
  • Extend NotifyModel.removeByApp to handle overlap-type notifications and log a warning when no matching notifications are found
  • Update translation files across all languages to include the new “Clean All” text and enforce proper XML encoding declarations

@wjyrich wjyrich requested a review from 18202781743 September 24, 2025 08:56
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.

@sourcery-ai
Copy link

sourcery-ai bot commented Sep 24, 2025

Reviewer's Guide

Implements a “Clean All” action for overlap notifications by adding a custom button component to the UI, extending the model’s removeByApp method to handle overlap-type notifications (with warning logs for missing apps), and updating translation files to include the new label.

Class diagram for updated OverlapNotify and NotifyModel

classDiagram
    class OverlapNotify {
        +NotifyModel notifyModel
        +var model
        +customButtonComponent: AnimationSettingButton
    }
    class NotifyModel {
        +removeByApp(appName)
        -Handles Group and Overlap types
        -Warning log for missing app
    }
    OverlapNotify --> NotifyModel: uses
    AnimationSettingButton <|-- OverlapNotify: customButtonComponent
Loading

File-Level Changes

Change Details Files
Introduce custom Clean All button for overlap notifications
  • Added notifyModel and model properties to OverlapNotify
  • Replaced onRemove handler with AnimationSettingButton invoking clean-all action
  • Exposed customButtonComponent in NotifyItemContent and used it in Loader
  • Defined defaultButtonComponent fallback in NotifyItemContent
  • Passed notifyModel to OverlapNotify in NotifyViewDelegate
panels/notification/center/OverlapNotify.qml
panels/notification/plugin/NotifyItemContent.qml
panels/notification/center/NotifyViewDelegate.qml
Extend removeByApp logic to support overlap notifications and add warning logs
  • Imported qlogging.h
  • Adjusted type check to include NotifyType::Overlap
  • Logged warning when no notifications found for app
panels/notification/center/notifymodel.cpp
Add translation entries for Clean All and update TS headers
  • Inserted OverlapNotify context with Clean All message in translation files
  • Updated XML headers to include encoding and DOCTYPE declaration
panels/notification/center/translations/org.deepin.ds.notificationcenter_ar.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_az.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_de.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_hu.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_sq.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_es.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_fi.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_fr.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_ja.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_lo.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_pl.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_pt_BR.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_uk.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_zh_HK.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_zh_TW.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_ca.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_zh_CN.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_bo.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_it.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_ko.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_nb_NO.ts
panels/notification/center/translations/org.deepin.ds.notificationcenter_ru.ts

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

@wjyrich wjyrich force-pushed the fix-bug-cleanAllNoti branch from f5836df to 904088f Compare September 24, 2025 09:33
@wjyrich wjyrich force-pushed the fix-bug-cleanAllNoti branch 4 times, most recently from 82c2f4f to 456ce25 Compare September 24, 2025 09:39
Added a "Clean All" button to overlap notification items to allow users
to remove all notifications from a specific app at once
The button is implemented as a custom button component in OverlapNotify
with proper translation support
Modified NotifyModel to handle removal of overlap type notifications and
added warning logging for missing apps
Updated translation files across multiple languages to include the new
"Clean All" text

feat: 为重叠通知添加清除全部按钮

在重叠通知项中添加"清除全部"按钮,允许用户一次性移除特定应用的所有通知
该按钮作为自定义按钮组件在 OverlapNotify 中实现,并支持多语言翻译
修改 NotifyModel 以处理重叠类型通知的移除,并为不存在的应用添加警告日志
更新了多个语言的翻译文件以包含新的"清除全部"文本
@wjyrich wjyrich force-pushed the fix-bug-cleanAllNoti branch from 456ce25 to 28329b6 Compare September 24, 2025 09:46
@deepin-ci-robot
Copy link

deepin pr auto review

根据提供的代码差异,我来对代码进行审查,并提出改进建议:

  1. 代码逻辑方面:
  • 在NotifyViewDelegate.qml中,删除通知的逻辑从使用model.id改为使用model.appName,这是一个合理的改进,因为通过应用名来删除通知更加准确和可靠。
  • 在OverlapNotify.qml中,添加了一个"Clean All"按钮,用于清理所有通知,这增强了用户体验。
  • 在notifymodel.cpp中,removeByApp函数的逻辑进行了优化,增加了对Overlap类型通知的处理,并改进了错误处理。
  1. 代码质量方面:
  • 代码结构清晰,职责分明。QML负责UI展示,C++负责业务逻辑。
  • 翻译文件的格式统一,添加了适当的编码声明。
  • 变量命名规范,如clearButton、clearLoader等。
  1. 代码性能方面:
  • notifymodel.cpp中的removeByApp函数在删除通知时使用了beginRemoveRows和endRemoveRows,确保了UI更新的性能。
  • 使用了deleteLater来安全地删除对象,避免了潜在的内存泄漏。
  1. 代码安全方面:
  • 在removeByApp函数中添加了错误检查,当找不到对应应用的通知时会输出警告信息。
  • 使用了Qt的信号槽机制来确保UI更新的安全性。

改进建议:

  1. 在OverlapNotify.qml中,可以考虑为"Clean All"按钮添加确认对话框,防止用户误操作删除所有通知。

  2. 在notifymodel.cpp中,可以考虑将通知类型检查使用枚举类或常量,而不是直接使用字符串比较,这样可以减少类型错误的可能性。

  3. 在删除通知的逻辑中,可以考虑添加事务处理,确保数据的一致性。例如,如果在删除过程中发生错误,可以回滚操作。

  4. 在翻译文件中,建议统一"Clear All"和"Clean All"的翻译,保持术语一致性。目前中文翻译已经统一为"清除全部",这是一个好的做法。

  5. 在NotifyItemContent.qml中,可以考虑为clearLoader添加加载状态的处理,避免在组件加载过程中出现UI闪烁。

  6. 建议添加单元测试,特别是对notifymodel.cpp中的删除功能进行测试,确保各种场景下都能正确处理。

  7. 考虑添加日志记录,记录通知的删除操作,便于后续的问题追踪和调试。

  8. 在删除大量通知时,可以考虑添加进度提示,提升用户体验。

总体来说,这次代码改进是合理的,增强了通知管理的功能,并提高了代码质量和安全性。上述建议可以根据实际需求和开发计划逐步实施。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@wjyrich
Copy link
Contributor Author

wjyrich commented Sep 25, 2025

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Sep 25, 2025

This pr force merged! (status: behind)

@deepin-bot deepin-bot bot merged commit c06f1bc into linuxdeepin:master Sep 25, 2025
10 of 12 checks passed
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