Skip to content

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented Oct 14, 2025

  1. Add fetchFirstEntity method to data accessor interfaces for retrieving earliest notifications
  2. Implement notification auto-cleanup feature that removes processed notifications older than configured days
  3. Add Timeout processed type for handling expired notifications
  4. Add configuration option for notification cleanup days with default value of 7 days
  5. Implement cleanup timer that runs every second to check for expired notifications
  6. Update notification processing to handle timeout closures properly

feat: 添加通知自动清理和超时处理功能

  1. 在数据访问器接口中添加 fetchFirstEntity 方法用于获取最早的通知
  2. 实现通知自动清理功能,删除超过配置天数的已处理通知
  3. 添加 Timeout 处理类型用于处理过期通知
  4. 添加通知清理天数的配置选项,默认值为7天
  5. 实现清理定时器,每秒检查一次过期通知
  6. 更新通知处理逻辑以正确处理超时关闭

PMS: BUG-284979

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 @wjyrich, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

m_systemApps = config->value("systemApps").toStringList();
// TODO temporary fix for AppNamesMap
m_appNamesMap = config->value("AppNamesMap").toMap();
m_cleanupDays = config->value("notificationCleanupDays", 7).toInt();
Copy link
Contributor

Choose a reason for hiding this comment

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

加个有效的检验,

1. Add fetchFirstEntity method to data accessor interfaces for
retrieving earliest notifications
2. Implement notification auto-cleanup feature that removes processed
notifications older than configured days
3. Add Timeout processed type for handling expired notifications
4. Add configuration option for notification cleanup days with default
value of 7 days
5. Implement cleanup timer that runs every second to check for expired
notifications
6. Update notification processing to handle timeout closures properly

feat: 添加通知自动清理和超时处理功能

1. 在数据访问器接口中添加 fetchFirstEntity 方法用于获取最早的通知
2. 实现通知自动清理功能,删除超过配置天数的已处理通知
3. 添加 Timeout 处理类型用于处理过期通知
4. 添加通知清理天数的配置选项,默认值为7天
5. 实现清理定时器,每秒检查一次过期通知
6. 更新通知处理逻辑以正确处理超时关闭

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

deepin pr auto review

代码审查报告

1. 整体功能

这段代码实现了一个通知系统的自动清理功能,主要添加了以下功能:

  • 新增了 fetchExpiredEntities 方法用于获取过期通知
  • 添加了通知清理天数配置项 notificationCleanupDays
  • 实现了定时清理过期通知的机制

2. 代码质量评估

优点:

  1. 代码结构清晰,遵循了现有的代码风格和模式
  2. 使用了适当的互斥锁保护共享资源
  3. 添加了必要的注释和文档
  4. 实现了完整的接口定义和实现

需要改进的地方:

  1. 错误处理不够完善

    • fetchExpiredEntities 方法中,数据库查询失败时只是打印警告并返回空列表,可以考虑添加重试机制或更详细的错误处理
  2. 性能考虑

    • fetchExpiredEntities 方法在内存实现中使用了全量遍历,当通知数量很大时可能会有性能问题
    • 建议为内存实现添加索引或缓存机制来优化查询性能
  3. 配置验证

    • notificationCleanupDays 配置没有进行范围验证,可能导致不合理值
    • 建议添加最小值和最大值的限制
  4. 定时器间隔

    • 清理定时器的间隔设置为1秒可能过于频繁,建议根据实际需求调整

3. 代码安全评估

  1. SQL注入防护

    • 数据库查询使用了参数化查询,有效防止了SQL注入风险
  2. 线程安全

    • 使用了QMutexLocker保护共享资源,确保了线程安全
    • 定时器的信号槽连接在构造函数中完成,需要注意对象的生命周期管理
  3. 潜在问题

    • onCleanupExpiredNotifications 中直接调用 notificationClosed 可能会导致递归调用,需要确保不会形成循环

4. 改进建议

  1. 性能优化
// 建议在 MemoryAccessor 中添加按时间索引的容器
QMap<qint64, NotifyEntity> m_entitiesByTime;

// 在 fetchExpiredEntities 中使用二分查找优化
auto it = m_entitiesByTime.upperBound(expiredTime);
expiredEntities.reserve(it - m_entitiesByTime.begin());
for (auto i = m_entitiesByTime.begin(); i != it; ++i) {
    expiredEntities.append(i.value());
}
  1. 配置验证
if (config->value("notificationCleanupDays").isValid()) {
    m_cleanupDays = qBound(1, config->value("notificationCleanupDays").toInt(), 30); // 限制在1-30天之间
}
  1. 错误处理增强
if (!query.exec()) {
    qWarning(notifyDBLog) << "Query execution error:" << query.lastError().text();
    // 可以考虑添加重试逻辑
    return {};
}
  1. 定时器间隔优化
m_cleanupTimer->setInterval(24 * 60 * 60 * 1000); // 改为每天执行一次
  1. 防止递归调用
void NotificationManager::onCleanupExpiredNotifications()
{    
    const qint64 cutoffTime = QDateTime::currentDateTime().addDays(-m_cleanupDays).toMSecsSinceEpoch();
    auto expiredEntities = m_persistence->fetchExpiredEntities(cutoffTime);
    
    // 临时禁用信号,防止递归调用
    bool wasBlocked = m_blockNotifications;
    m_blockNotifications = true;
    
    for (const auto &entity : expiredEntities) {
        notificationClosed(entity.id(), entity.bubbleId(), NotifyEntity::Timeout);
    }
    
    m_blockNotifications = wasBlocked;
}

5. 总结

整体实现思路清晰,功能完整,但在性能优化和错误处理方面还有改进空间。建议按照上述建议进行优化,特别是在处理大量通知时的性能表现和错误恢复机制。同时,建议添加单元测试来验证新功能的正确性和稳定性。

m_appNamesMap = config->value("AppNamesMap").toMap();

if(!config->value("notificationCleanupDays").isNull()) {
m_cleanupDays = config->value("notificationCleanupDays").toInt();
Copy link
Contributor

Choose a reason for hiding this comment

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

这个时间,可能是个无效的,

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

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

@deepin-bot
Copy link

deepin-bot bot commented Oct 16, 2025

TAG Bot

New tag: 2.0.14
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #1303

@wjyrich
Copy link
Contributor Author

wjyrich commented Oct 22, 2025

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Oct 22, 2025

This pr force merged! (status: behind)

@deepin-bot deepin-bot bot merged commit 61b256c into linuxdeepin:master Oct 22, 2025
9 of 10 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