GNOME Bugzilla – Bug 710596
Fix fallout from notification changes
Last modified: 2013-10-30 15:41:07 UTC
See patch.
Created attachment 257801 [details] [review] Move helper function Commit 5f081b8f8d5c4 moved the code which uses it to notifictionDaemon, but forgot to move the function as well.
Review of attachment 257801 [details] [review]: We can simply replace this with str.endsWith().
Created attachment 257802 [details] [review] Fix fallout from notification changes
Created attachment 257803 [details] [review] notificationDaemon: Pass the correct id to makeButton() The function expects the action's ID, not the notification's one.
Review of attachment 257802 [details] [review]: OK.
Review of attachment 257803 [details] [review]: Yes.
Created attachment 257884 [details] [review] NotificationDaemon: fix more fallout Missing Gtk import for action-icon buttons.
Review of attachment 257884 [details] [review]: Yup.
Created attachment 258152 [details] [review] notificationDaemon: Unpack button label deep_unpack() doesn't unpack as deeply as one might hope ...
Created attachment 258153 [details] [review] notificationDaemon: Fix button parameter name Gio ended up using 'target' rather than 'action-target'.
Attachment 257802 [details] pushed as 4b09d57 - Fix fallout from notification changes Attachment 257803 [details] pushed as 61c5b8e - notificationDaemon: Pass the correct id to makeButton()
Review of attachment 258152 [details] [review]: What type is buttons? a{sv}? Should we have a comment for documentation purposes?
Review of attachment 258153 [details] [review]: ::: js/ui/notificationDaemon.js @@ +760,2 @@ _onButtonClicked: function(button) { + let { "action": action, "target": actionTarget } = button; Single quotes for non translatable strings?
Comment on attachment 257884 [details] [review] NotificationDaemon: fix more fallout Attachment 257884 [details] pushed as eb66407 - NotificationDaemon: fix more fallout
Review of attachment 258152 [details] [review]: buttons is an a{sv} yes. It's somewhat documented here: https://wiki.gnome.org/GNotification
Review of attachment 258153 [details] [review]: We need to fix https://wiki.gnome.org/GNotification then...
Created attachment 258206 [details] [review] notificationDaemon: Fix urgency hint We currently mark notifications as urgent which merely contain the 'urgent' property, even when set to false. Look at the actual value instead.
Created attachment 258207 [details] [review] notificationDaemon: Fix custom icons The 'icon' property contains a serialized GIcon, so we need to deserialize it when setting the icon.
Attachment 258152 [details] pushed as dac513e - notificationDaemon: Unpack button label Attachment 258153 [details] pushed as 34e75fc - notificationDaemon: Fix button parameter name
Review of attachment 258206 [details] [review]: Yep.
Review of attachment 258207 [details] [review]: Yep.
Created attachment 258288 [details] [review] notificationDaemon: Save notifications on source destruction While the existing comment is correct in that a source's notifications will be destroyed first, the code takes a shortcut which prevents the Source::count-updated signal from being emitted. Given that the purpose of the signal is to keep notification counters up-to-date which is pointless when the source is about to be destroyed, the shortcut makes sense; just save notifications explicitly in that case.
Created attachment 258289 [details] [review] messageTray: Emit Source::count-updated before destruction It isn't useful to emit the 'count-updated' signal for every single notification when a source is destroyed, so the code takes a shortcut to prevent this. However the new persistence code in notificationDaemon now relies on the signal to be emitted once all notifications have been destroyed, so do this.
Only one of attachment 258288 [details] [review] and attachment 258289 [details] [review] is needed - I'd go with the former, but no strong preference.
Attachment 258206 [details] pushed as 04d28a0 - notificationDaemon: Fix urgency hint Attachment 258207 [details] pushed as 41315f4 - notificationDaemon: Fix custom icons
I don't understand. https://git.gnome.org/browse/gnome-shell/tree/js/ui/messageTray.js#n1368 This calls countUpdated when a notification is destroyed, and when the source is destroyed: https://git.gnome.org/browse/gnome-shell/tree/js/ui/messageTray.js#n1407 We destroy all notifications. Where is this shortcut?
Review of attachment 258288 [details] [review]: oh, grr, nevermind, I just noticed it. Yeah, let's do this.
Attachment 258288 [details] pushed as 633dd0d - notificationDaemon: Save notifications on source destruction