Commit d8e0071f authored by Kai Uwe Broulik's avatar Kai Uwe Broulik 🍇

[Notifications] Compress removal of notifications

When removing a notification, it is removed from the model, which will cause the
view to update and move up an older notification, if any. If you now delete a bunch
of notifications in quick succession we will create a dialog every time which then
gets deleted again, causing another notification to move up, etc causing high CPU
spikes and plasma freezes.

This patch compresses removing notifications and tries to announce consecuetive
ranges of notifications to be removed in one go.

BUG: 423594
FIXED-IN: 5.21.0
parent 7702694d
......@@ -29,7 +29,6 @@
#include <QDebug>
#include <QProcess>
#include <QTimer>
#include <KShell>
......@@ -44,7 +43,21 @@ AbstractNotificationsModel::Private::Private(AbstractNotificationsModel *q)
: q(q)
, lastRead(QDateTime::currentDateTimeUtc())
{
pendingRemovalTimer.setSingleShot(true);
pendingRemovalTimer.setInterval(50);
connect(&pendingRemovalTimer, &QTimer::timeout, q, [this, q] {
QVector<int> rowsToBeRemoved;
rowsToBeRemoved.reserve(pendingRemovals.count());
for (uint id : qAsConst(pendingRemovals)) {
int row = q->rowOfNotification(id); // oh the complexity...
if (row == -1) {
continue;
}
rowsToBeRemoved.append(row);
}
removeRows(rowsToBeRemoved);
});
}
AbstractNotificationsModel::Private::~Private()
......@@ -125,11 +138,15 @@ void AbstractNotificationsModel::Private::onNotificationRemoved(uint removedId,
return;
}
// Otherwise if explicitly closed by either user or app, remove it
// Otherwise if explicitly closed by either user or app, mark it for removal
// some apps are notorious for closing a bunch of notifications at once
// causing newer notifications to move up and have a dialogs created for them
// just to then be discarded causing excess CPU usage
pendingRemovals.append(removedId);
q->beginRemoveRows(QModelIndex(), row, row);
notifications.removeAt(row);
q->endRemoveRows();
if (!pendingRemovalTimer.isActive()) {
pendingRemovalTimer.start();
}
}
void AbstractNotificationsModel::Private::setupNotificationTimeout(const Notification &notification)
......@@ -158,6 +175,50 @@ void AbstractNotificationsModel::Private::setupNotificationTimeout(const Notific
timer->start();
}
void AbstractNotificationsModel::Private::removeRows(const QVector<int> &rows)
{
if (rows.isEmpty()) {
return;
}
QVector<int> rowsToBeRemoved(rows);
std::sort(rowsToBeRemoved.begin(), rowsToBeRemoved.end());
QVector<QPair<int, int>> clearQueue;
QPair<int, int> clearRange{rowsToBeRemoved.first(), rowsToBeRemoved.first()};
for (int row : rowsToBeRemoved) {
if (row > clearRange.second + 1) {
clearQueue.append(clearRange);
clearRange.first = row;
}
clearRange.second = row;
}
if (clearQueue.isEmpty() || clearQueue.last() != clearRange) {
clearQueue.append(clearRange);
}
int rowsRemoved = 0;
for (int i = clearQueue.count() - 1; i >= 0; --i) {
const auto &range = clearQueue.at(i);
q->beginRemoveRows(QModelIndex(), range.first, range.second);
for (int j = range.second; j >= range.first; --j) {
notifications.removeAt(j);
++rowsRemoved;
}
q->endRemoveRows();
}
Q_ASSERT(rowsRemoved == rowsToBeRemoved.count());
pendingRemovals.clear();
}
int AbstractNotificationsModel::rowOfNotification(uint id) const
{
auto it = std::find_if(d->notifications.constBegin(), d->notifications.constEnd(), [id](const Notification &item) {
......@@ -325,45 +386,17 @@ void AbstractNotificationsModel::clear(Notifications::ClearFlags flags)
return;
}
// Tries to remove a contiguous group if possible as the likely case is
// you have n unread notifications at the end of the list, we don't want to
// remove and signal each item individually
QVector<QPair<int, int>> clearQueue;
QVector<int> rowsToRemove;
QPair<int, int> clearRange{-1, -1};
for (int i = d->notifications.count() - 1; i >= 0; --i) {
for (int i = 0; i < d->notifications.count(); ++i) {
const Notification &notification = d->notifications.at(i);
bool clear = (flags.testFlag(Notifications::ClearExpired) && notification.expired());
if (clear) {
if (clearRange.second == -1) {
clearRange.second = i;
}
clearRange.first = i;
} else {
if (clearRange.first != -1) {
clearQueue.append(clearRange);
clearRange.first = -1;
clearRange.second = -1;
}
if (flags.testFlag(Notifications::ClearExpired) && notification.expired()) {
rowsToRemove.append(i);
}
}
if (clearRange.first != -1) {
clearQueue.append(clearRange);
clearRange.first = -1;
clearRange.second = -1;
}
for (const auto &range : clearQueue) {
beginRemoveRows(QModelIndex(), range.first, range.second);
for (int i = range.second; i >= range.first; --i) {
d->notifications.removeAt(i);
}
endRemoveRows();
}
d->removeRows(rowsToRemove);
}
void AbstractNotificationsModel::onNotificationAdded(const Notification &notification)
......
......@@ -78,6 +78,8 @@ protected:
private:
friend class NotificationTest;
class Private;
QScopedPointer<Private> d;
......
......@@ -25,6 +25,9 @@
#include "server.h"
#include <QDateTime>
#include <QTimer>
class QTimer;
namespace NotificationManager
{
......@@ -41,6 +44,8 @@ public:
void setupNotificationTimeout(const Notification &notification);
void removeRows(const QVector<int> &rows);
AbstractNotificationsModel *q;
QVector<Notification> notifications;
......@@ -49,6 +54,9 @@ public:
// an app might wait indefinitely for the notification to do so
QHash<uint /*notificationId*/, QTimer*> notificationTimeouts;
QVector<uint /*notificationId*/> pendingRemovals;
QTimer pendingRemovalTimer;
QDateTime lastRead;
};
......
/*
* Copyright (C) 2017 David Edmundson <davidedmundson@kde.org>
* Copyright (C) 2020 Kai Uwe Broulik <kde@broulik.de>
*
* This program is free software you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
......@@ -22,6 +23,10 @@
#include <QDebug>
#include "notification.h"
#include "notificationsmodel.h"
#include "server.h"
namespace NotificationManager {
class NotificationTest : public QObject
{
......@@ -31,6 +36,8 @@ public:
private Q_SLOTS:
void parse_data();
void parse();
void compressNotificationRemoval();
};
void NotificationTest::parse_data()
......@@ -85,6 +92,64 @@ void NotificationTest::parse()
QCOMPARE(notification.body(), expectedOut);
}
QTEST_GUILESS_MAIN(NotificationTest)
void NotificationTest::compressNotificationRemoval()
{
const int notificationCount = 10;
const int gapId = 4;
auto model = NotificationsModel::createNotificationsModel();
QSignalSpy rowsRemovedSpy(model.data(), &QAbstractItemModel::rowsRemoved);
QVERIFY(rowsRemovedSpy.isValid());
for (uint i = 1; i <= notificationCount; ++i) {
Notification notification{i};
notification.setSummary(QStringLiteral("Notification %1").arg(i));
model->onNotificationAdded(notification);
}
QCOMPARE(model->rowCount(), notificationCount);
for (uint i = 1; i <= notificationCount; ++i) {
// Leave a gap inbetween
if (i != gapId) {
model->onNotificationRemoved(i, Server::CloseReason::Revoked);
}
}
// We should have two ranges that we ended up removing
QTRY_COMPARE(rowsRemovedSpy.count(), 2);
// The fact that it emits row removal in reverse order is an implementation detail
// We only really care that the number of rows emitted matches our expectation
int removedCount = 0;
for (const auto &removedEmission : rowsRemovedSpy) {
const int from = removedEmission.at(1).toInt();
const int to = removedEmission.at(2).toInt();
removedCount += (to - from) + 1;
}
QCOMPARE(removedCount, notificationCount - 1);
QCOMPARE(model->rowCount(), 1);
rowsRemovedSpy.clear();
// Removing a random non-existing notification should noop
model->onNotificationRemoved(3, Server::CloseReason::Revoked);
QTRY_COMPARE(rowsRemovedSpy.count(), 0);
QCOMPARE(model->rowCount(), 1);
rowsRemovedSpy.clear();
// Now remove the last one
model->onNotificationRemoved(gapId, Server::CloseReason::Revoked);
QTRY_COMPARE(rowsRemovedSpy.count(), 1);
QCOMPARE(rowsRemovedSpy.at(0).at(1), 0); // from
QCOMPARE(rowsRemovedSpy.at(0).at(2), 0); // to
QCOMPARE(model->rowCount(), 0);
}
} // namespace NotificationManager
QTEST_GUILESS_MAIN(NotificationManager::NotificationTest)
#include "notifications_test.moc"
......@@ -25,7 +25,7 @@
namespace NotificationManager {
class NotificationsModel : public AbstractNotificationsModel
class Q_DECL_EXPORT NotificationsModel : public AbstractNotificationsModel
{
public:
using Ptr = QSharedPointer<NotificationsModel>;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment