GNOME Bugzilla – Bug 710115
More message tray cleanups
Last modified: 2014-01-17 14:13:21 UTC
At the Montreal Summit, we want to land the new GNotification API which will use a new notification specification. Do the usual cleanup work before landing this.
Created attachment 257265 [details] [review] messageTray: Clean up code that determines if something is clearable
Created attachment 257266 [details] [review] messageTray: Remove transient sources As far as I can tell, the only behavior change of a transient source is that they auto-destroy after viewing their summary box pointer. Since all transient sources are only associated with transient notifications, it seems that we can never get to their summary box pointer in the first place! Remove support for this.
Created attachment 257267 [details] [review] messageTray: Move the notification policy classes here The NotificationDaemon really should be for the hookup of remote notifications, rather than policies.
Created attachment 257268 [details] [review] messageTray: Only connect to a notification's open/destroy once If we pushNotification the same notification multiple times, we won't append it to the array again, but we will attach multiple handlers needlessly.
Created attachment 257269 [details] [review] messageTray: Split out the notification's destroy handler This is complex enough to split out.
Created attachment 257270 [details] [review] messageTray: Don't remove and re-add the focus group on button changes
Created attachment 257271 [details] [review] messageTray: Remove reNotifyAfterHideNotification logic When we hide the summary box pointer, we always drop to the tray instead of the desktop, so this code would simply drop the notification anyway.
Review of attachment 257265 [details] [review]: ::: js/ui/messageTray.js @@ +1546,3 @@ this._clearItem = this.addAction(_("Clear Messages"), function() { + let toDestroy = tray.getSources().filter(function(source) { + return source.isClearable(); Not a function call.
Review of attachment 257266 [details] [review]: The analysis is wrong, a summary box pointer can appear for a transient source if a transient notification is emitted while interacting with the message tray, so this is a behavior change. Also, this merges transient notifications with resident and persistent ones, which is not quite what the commit message says.
Review of attachment 257267 [details] [review]: I thought the split was "NotificationDaemon for application notifications, MessageTray internal API", but ok for me either way.
Review of attachment 257268 [details] [review]: Yes
Review of attachment 257269 [details] [review]: Sure
Review of attachment 257270 [details] [review]: Ok
Review of attachment 257271 [details] [review]: No, it would put the notification in the queue after the end of the animation, which seems more correct that acknowledging the notification without the user seeing it.
(In reply to comment #9) > The analysis is wrong, a summary box pointer can appear for a transient source > if a transient notification is emitted while interacting with the message tray, > so this is a behavior change. That is true, but the notification will be queued while the tray was up, and will display after it collapses > Also, this merges transient notifications with resident and persistent ones, > which is not quite what the commit message says. Yes, we drop the code that creates new sources if the app already exists. I'm not aware of any apps sending transient notifications -- they should simply be for system sources, so I'm not sure why that matters. Keep in mind that I don't remove transient notifications, only transient sources.
Attachment 257265 [details] pushed as da14e2c - messageTray: Clean up code that determines if something is clearable Attachment 257267 [details] pushed as 66a4cb5 - messageTray: Move the notification policy classes here Attachment 257268 [details] pushed as 99cf4e5 - messageTray: Only connect to a notification's open/destroy once Attachment 257269 [details] pushed as 25fd23e - messageTray: Split out the notification's destroy handler Attachment 257270 [details] pushed as 96aa33f - messageTray: Don't remove and re-add the focus group on button changes Let's push all of these for now. Fixed the issue with isClearable in the first patch.
Comment on attachment 257266 [details] [review] messageTray: Remove transient sources Attachment 257266 [details] pushed as b52e74b - messageTray: Remove transient sources