Bug 440051 - Reusing replaces_id after expiry does not work
Summary: Reusing replaces_id after expiry does not work
Status: RESOLVED NOT A BUG
Alias: None
Product: plasmashell
Classification: Plasma
Component: Notifications (show other bugs)
Version: 5.22.3
Platform: Manjaro Linux
: NOR normal
Target Milestone: 1.0
Assignee: Kai Uwe Broulik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-19 18:29 UTC by Krut Patel
Modified: 2021-07-22 11:46 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Krut Patel 2021-07-19 18:29:47 UTC
SUMMARY
Ever since the update to 5.22.x, notifications that reuse a replaces_id by setting it equal to a notif_id from notification that has already expired (aka, timed out and no longer visible), are not rendered.

STEPS TO REPRODUCE
1. Use https://github.com/phuhl/notify-send.py to create a notification: 
    notify-send.py Body -r 1234 -a Appname
2. Wait for the notification to timeout
3. Resend the notification, with a modified body
    notify-send.py NewBody -r 1234 -a Appname

OBSERVED RESULT
Modified notification does not show up.

EXPECTED RESULT
Modified notification should show up as if it were a new notification (with the default timeout).

SOFTWARE/OS VERSIONS
Linux: Manjaro
KDE Plasma Version: 5.22.3
KDE Frameworks Version: 5.84.0
Qt Version: 5.15.2

ADDITIONAL INFORMATION
This scenario (reusing replaces_id after expiry) is not specified by the spec, but virtually every notification server behaves as expected, by creating a new notification.
I guess the buggy behaviour was introduced in the commits https://invent.kde.org/plasma/plasma-workspace/-/commit/ee822faf1c86844b6bcac8f1c55cc152de2c376e and https://invent.kde.org/plasma/plasma-workspace/-/commit/5890aa64f939285486f3d64caf46de13a8e2dd62
Shouldn't the notifications be getting cleaned up on expiry?
Comment 1 Kai Uwe Broulik 2021-07-19 18:49:12 UTC
The spec says "The server must atomically (ie with no flicker or other visual cues) replace the given notification with this one."
Comment 2 David Edmundson 2021-07-19 22:34:58 UTC
So resolved not a bug?
Comment 3 Krut Patel 2021-07-22 06:10:34 UTC
> The spec says "The server must atomically (ie with no flicker or other visual cues) replace the given notification with this one."

I understand that, but I believe they were referring to notifications that were actually _visible_ on the screen.

I've looked into other notification daemons, and all of those (as far as I know) assume that if the replaces_id is for an expired notification, the user means to display a new notification altogether.
See https://gitlab.gnome.org/Archive/notification-daemon/-/blob/master/src/nd-daemon.c#L205

Here are my doubts:
1. Is there a way to make the notification visible again after it has expired? If so, the user can explicitly use that instead of relying on unspecified behaviour.
2. More generally, what is the need to keep around a notification that has expired, and which has _not_ been marked persistent/resident? If the normal notifications are garbage-collected on expiry, https://invent.kde.org/plasma/plasma-workspace/-/blob/ee822faf1c86844b6bcac8f1c55cc152de2c376e/libnotificationmanager/abstractnotificationsmodel.cpp#L96 would get activated and give the desired behaviour.

The reason I would like to have this behaviour, is that it greatly simplifies the client code when they want to display only one notification at any given time. They can simply store the id of the last notif they displayed and use that  as replaces_id, without having to keep track of whether the previous notif has timed out or not.

And sorry for not replying sooner - didn't get any emails of this thread for some reason.
Comment 4 Krut Patel 2021-07-22 06:20:11 UTC
On reading the spec further, it also mentions under the NotificationClosed signal section:
"The ID specified in the signal is invalidated before the signal is sent and may not be used in any further communications with the server."

So it seems setting replaces_id as ID of an expired notification is not allowed by the spec. I guess it would be better to mark this as notabug, and have the client deal with the extra bookkeeping.
Comment 5 David Edmundson 2021-07-22 11:46:43 UTC
I would agree with that last part, sorry it puts more burden on you.