GNOME Bugzilla – Bug 611611
combined notifications
Last modified: 2011-03-22 22:50:14 UTC
if you have multiple un-acknowledged notifications from a given source, they should merge together as seen in http://www.gnome.org/~mccann/screenshots/clips/20100122204135/shell-mockup-mail-expanded.png and http://www.gnome.org/~mccann/screenshots/clips/20100122204135/shell-mockup-mail-banner.png In particular, the icon (in both summary and notification banner view) should get a numeric badge showing how many un-acknowledged notifications there are, and when you expand a new notification, it should show the stack of previous notifications. It is possible that this behavior only happens for certain notification types. We have not figured that out in detail.
Created attachment 183163 [details] [review] Work-in-progress stacked notifications I'm attaching this now as several people have asked me about it. This is rebased to latest and somewhat works in that it stacks notifications, but then there are errors I didn't have time to debug yet. It should be pretty close though. Unfortunately, I'm going away for the weekend and there are other things I need to do by Tuesday (like conference proposals submissions, OPW program announcement if we are doing the next round), so I will only likely be able to work on this next on Tuesday.
The two other things that this feature needs that I discussed with Jon, but that are not addressed in the patch are: 1) hiding icons in all notifications in the stack other than the first one 2) removing individual scrollbars in all notifications, and using a single scrollbar for the stack (e.g. try libnotify/tests/test-basic.c)
Created attachment 184010 [details] [review] MessageTray: show multiple notifications per source Add an ability to show multple notifications per source, so that the user doesn't miss seeing any notifications. This patch is ready for review. I'll work on the remaining two features (hiding all but first notification icons and using a single scrollbar) and attach them as separate patches.
Created attachment 184012 [details] [review] MessageTray: show multiple notifications per source Fixed trailing whitespaces.
Created attachment 184037 [details] [review] MessageTray: show multiple notifications per source Updated to apply to master.
Created attachment 184038 [details] [review] MessageTray: show multiple notifications per source Fix the loop that destroys notifications to make sure it destroys all of them. Only show the icon for the first notification in the stack.
Created attachment 184039 [details] [review] MessageTray: use single scrollbar for summary notification stacks We want to allow the user to scroll through all notifications from source by using a single scrollbar. We suppress the individual scrollbars inside the notifications. As one exception, we keep the original scrollbar for chat notifications because it has a distinct look, ending above the text entry box.
Comment on attachment 184038 [details] [review] MessageTray: show multiple notifications per source The mockups have a faint outline around each notification, which makes it clear where the boundaries between individual notifications are. It would be nice to have that here. When you click to dispose a stack, some of the individual notifications disappear before the boxpointer does. >+ showIcon: function(showIcon) { call it setIconVisible or something. It's confusing to have a boolean that means "do the opposite of the function name". (And for that matter, you can just do "this._icon.visible = visible;" which ClutterActor will turn into a show or hide for you.) >- _notificationRemoved: function() { >+ _lastNotificationRemoved: function() { extra space before the function name there >+ doneShowingNotificationStack: function() { >+ let notificationActors = this.notificationStack.get_children(); >+ for (let i = 0; i < notificationActors.length; i++) { >+ notificationActors[i]._delegate.collapseCompleted(); >+ notificationActors[i]._delegate.disconnect(this._notificationExpandedIds[i]); >+ notificationActors[i]._delegate.disconnect(this._notificationDoneDisplayingIds[i]); >+ notificationActors[i]._delegate.disconnect(this._notificationDestroyedIds[i]); >+ this.notificationStack.remove_actor(notificationActors[i]); >+ notificationActors[i]._delegate.showIcon(true); >+ } This is pretty messy. It would be cleaner to have the summaryitem keep an array of objects, with fields for the notification, its actor, and each signal id. >+ // this._summaryBoxPointer.actor.disconnect(this._summaryBoxPointerAllocationChangedId); >+ // this._summaryBoxPointerAllocationChangedId = 0; ? Other than the bug mentioned at the beginning, I don't see anything obviously wrong with the patch, but I'm dubious about changing this much of the notification code at the last minute... Not my call though.
Created attachment 184120 [details] [review] MessageTray: show multiple notifications per source Addresses simpler comments from Dan and removes a bug in filter() call.
Created attachment 184123 [details] [review] MessageTray: show multiple notifications per source Destroy all non-resident notifications on a clicked source without waiting for the application to get focused and destroy them so that all notifcations are removed at the same time. Just adds this.destroyNonResidentNotifications() call to open() in notificationDaemon.js .
Created attachment 184126 [details] [review] MessageTray: use single scrollbar for summary notification stacks Updated to apply on top of the previous patch.
Comment on attachment 184126 [details] [review] MessageTray: use single scrollbar for summary notification stacks this: >+ this.notificationStackView.vscroll.adjustment.connect('changed', Lang.bind(this, function(adjustment) { >+ let currentValue = adjustment.value + adjustment.page_size; >+ if (currentValue == this._oldMaxScrollAdjustment) >+ this.scrollTo(St.Side.BOTTOM); >+ this._oldMaxScrollAdjustment = adjustment.upper; >+ })); should make this: > _notificationAddedToSource: function(source, notification) { >- if (this.notificationStack.mapped) >+ if (this.notificationStack.mapped) { > this._appendNotificationToStack(notification); >+ this.scrollTo(St.Side.BOTTOM); >+ } > }, unnecesary, right? Otherwise looks good.
Comment on attachment 184123 [details] [review] MessageTray: show multiple notifications per source yup, only minor differences from previously-reviewed version
Created attachment 184131 [details] [review] MessageTray: use single scrollbar for summary notification stacks Remove unnecessary code per Dan's comment.