Active notification count incorrect when updating existing notifications

[mick at sayson.com]

Hi,

I believe I've found a bug in snorenotify 0.7.0 that's inhibiting me from
re-using existing notifications effectively.

The symptom I'm seeing is that I cannot update a notification more than 2
times before it stops issuing the updates I requested.

I have a piece of code that does the following

    bool needsNewNotification = !lastNotification.isValid() ||
lastNotification.closeReason() != Snore::Notification::CloseReasons::None;

    if (needsNewNotification) {
        qDebug() << "creating new notification";
        lastNotification = Snore::Notification{snoreApp, Snore::Alert(),
title, message, snoreIcon, 0};
    }
    else {
        qDebug() << "reusing last notification";
        lastNotification = Snore::Notification{lastNotification, title,
message, snoreIcon, 0};
    }

    notifyCore.broadcastNotification(lastNotification);

The rest isn't super important, the point here is that we try to reuse a
notification when it's valid and hasn't been closed.

I looked into the snorenotify source and it looks like snorenotify is
incrementing the active count on notification updates. In
snoreCore::broadcastNotification m_activeNotifications is checked to see if
it contains under 3 messages. If it does it adds the new notification to
the queue until the previous ones have been closed.

void SnoreCore::broadcastNotification(Notification notification)
{
    Q_D(SnoreCore);
    if (d->m_activeNotifications.size() > d->maxNumberOfActiveNotifications()) {
        qCDebug(SNORE) << "queue size:" << d->m_notificationQue.size()
<< "active size:" << d->m_activeNotifications.size();
        d->m_notificationQue.append(notification);
        return;
    }
    Q_ASSERT_X(!notification.data()->isBroadcasted(), Q_FUNC_INFO,
"Notification was already broadcasted.");
    qCDebug(SNORE) << "Broadcasting" << notification << "timeout:" <<
notification.timeout();
    if (d->m_notificationBackend != nullptr) {
        if (notification.isUpdate() &&
!d->m_notificationBackend->canUpdateNotification()) {
            requestCloseNotification(notification.old(),
Notification::Replaced);
        }
    }
    notification.data()->setBroadcasted();
    notification.addActiveIn(this);
    if (!d->m_notificationBackend) {
        d->startNotificationTimeoutTimer(notification);
    }
    emit d->notify(notification);
}


The problem is that if the backend supports updating notifications nothing
ties back the notification to the old one. In Notification::addActiveIn we
add a notification with a new Id unconditionally. When creating a
notification based on a previous one we always get a new ID

Snore::NotificationData::NotificationData(const Notification &old,
const QString &title, const QString &text, const Icon &icon, int
timeout, Notification::Prioritys priority):
    m_id(m_idCount++),
    m_timeout(priority == Notification::Emergency ? 0 : timeout),
    m_application(old.application()),
    m_alert(old.alert()),
    m_title(title),
    m_text(text),
    m_icon(icon),
    m_priority(priority),
    m_hints(m_application.constHints()),
    m_toReplace(old)
{
    notificationCount++;
    qCDebug(SNORE) << "Creating Notification: ActiveNotifications" <<
notificationCount << "id" << m_id;
    qCDebug(SNORE) << title << text;
}

Which is added to the map unconditionally at

void Notification::addActiveIn(const QObject *o)
{
    bool contains = d->m_activeIn.contains(o);
    Q_ASSERT_X(!contains, Q_FUNC_INFO, "already active");
    if (contains) {
        qCWarning(SNORE) << o << "already active in" << id();
        return;
    }
    d->m_activeIn.insert(o);
    SnoreCorePrivate::instance()->m_activeNotifications[id()] = *this;
    qCDebug(SNORE) <<
SnoreCorePrivate::instance()->m_activeNotifications.size();
}


I tried to work around it by manually issuing a close request myself and
only handling close requests on my end if closeReason() != Replaced,
however the closeReason() method is non-const, so I cannot check the
closeReason() in the notificationClosed callback.

Thanks for your time,
MIck Sayson
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/snorenotify/attachments/20200312/810df9a3/attachment.htm>