GNOME Bugzilla – Bug 775149
Replacing notifications by ID is broken: every 2nd notification cancels the last but is never shown itself
Last modified: 2016-12-12 14:56:28 UTC
Created attachment 340806 [details] test case I apologise in advance if this ends up being filed in the wrong place, but gnome-shell seems like the best 1st stop. Also, if I've missed any way to debug this, please let me know, and I'll run that. The documentation for GApplication states the following: > If a previous notification was sent with the same id, > it will be replaced with notification and shown again > as if it was a new notification. However, this does not seem to be the case. I have distilled the relevant part of my actual program into the attached test case and will now describe it. I'm sending consecutive notifications with the same id. But instead of the new notification replacing the old one, the 2nd send event just clears the old notification and doesn't show the new. Only every 2nd notification I send with the same id actually gets shown. It seems the delete side of the 'replacement' works, as the 1st notification disappears from below the top bar and in the message tray, but it's never actually replaced by the new notification, so the 2nd notification is never shown. Until... I fire a 3rd notification, then that shows up - and so on, odd/even/odd/even/etc. This seems broken and makes notifications unusable for my current case. Any ideas? The bizarre thing is that, if we have 2 different notifications in the mix, then by firing notification A, then B, then A... the replacement _does_ start to work for notification A (once B times out). But clearly this should not be required. Thanks!
(In reply to djb from comment #0) > > The bizarre thing is that, if we have 2 different notifications in the mix, > then by firing notification A, then B, then A... the replacement _does_ > start to work for notification A (once B times out). But clearly this should > not be required. this can be 're-broken' by clearing the notifications in the message tray. Then e.g. even if notification A had started working (because B had been sent) after the clear, only every 2nd (odd) one will actually be shown again.
Created attachment 340851 [details] [review] notificationDaemon: Keep source alive when replacing notification When a GNotification has the same ID as a previous one, it should be shown as a new notification after withdrawing the old one. However for this to work, we must not destroy the corresponding source if withdrawing the old notification lets the notification count drop to zero, so make sure the source is kept alive until the replace operation has completed.
Review of attachment 340851 [details] [review]: ::: js/ui/notificationDaemon.js @@ +729,2 @@ if (this._notifications[notificationId]) this._notifications[notificationId].destroy(); why don't you splice this notification out to a local variable and destroy it only after adding the new instance ? btw, this code seems different from what's in master so this patch doesn't apply
Created attachment 340910 [details] [review] notificationDaemon: Keep source alive when replacing notification (In reply to Rui Matos from comment #3) > why don't you splice this notification out to a local variable and destroy > it only after adding the new instance ? I played around with that, but it would require more code to make sure we don't accidentally destroy/delete/remove the updated notification - one example is the 'destroy' handler that's connected in addNotification() itself, but there were more places would be affected by switching around the ordering (mainly the rate-limiting of banners/notifications-per-source). > btw, this code seems different from what's in master so this patch doesn't > apply Ugh yeah, I missed a "stash" commit I had done earlier. Fixed now.
Review of attachment 340910 [details] [review]: ok
Attachment 340910 [details] pushed as accd24e - notificationDaemon: Keep source alive when replacing notification
Thanks for the quick fix. I presume it'll land in the next 3.22 point release too?
(In reply to Daniel Boles from comment #7) > I presume it'll land in the next 3.22 point release too? It wasn't on the 3-22 branch until now, so thanks for the reminder.