GNOME Bugzilla – Bug 649356
Implement the "Starburst" in the message tray.
Last modified: 2011-07-13 18:49:29 UTC
The mockup at http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-summary.png shows a small little counter that's presumably for unread messages. Implement the UI part of this.
Created attachment 187172 [details] [review] messageTray: first hack at a starburst count
oh, I didn't really hook any sources up other than the initial StarburstTestSource... I just fired up lg and typed 'imports.ui.messageTray.testStarburst()', restart, rinse, repeat.
Created attachment 187336 [details] [review] messageTray: second hack at a starburst count
this is a bit different design, but adapting it, it could work too (and it's a lot cleaner) works a lot better with the patches from bug 649513
This is nice. I started implementing the same thing for telepathyClient recently but did it using GtkEnumerableIcon -- this is nicer though. I'll dump my patch and wait for this to be merged then will cook up a new patch.
Needs a better name. "Starburst" implies something like ✸. I don't really like the "border-radius: 300px;" in the second patch... that's clearly a hack. Does ".5em" not dtrt? I guess the second patch does ovals with multi-digit counts, whereas the first just does a bigger circle? Oval is definitely better. I think the second patch also makes it easier to have a different-colored border as well, if we decide we want that.
Created attachment 189257 [details] [review] messageTray: Add an enumerable count for sources to use. This is just a proof of concept. The harder questions like "how do I know what Source I am", "how can I update the Source's notification count without creating a Notification (or updating an existing current one) and showing it", and "if the user can read and remove notifications, how can I have permanent actions and a count in the message tray" are still unanswered.
Created attachment 189258 [details] [review] notificationDaemon: Add a new notification hint 'count' This allows clients without any shell code to embed a notification counter as a hint. "Starburst" is no more! It's now "enumerable count".
er, I got the commentary mixed up. The "Source" answer was obviously meant for the notificationDaemon, and the s/Starburst/enumerable count/ the messageTray. (In reply to comment #6) > Needs a better name. "Starburst" implies something like ✸. If you don't like "count", suggest your own :D > I don't really like the "border-radius: 300px;" in the second patch... that's > clearly a hack. Does ".5em" not dtrt? It doesn't get the effect I was going for, but it works. '1em' works better. 'rt' is a godse > I guess the second patch does ovals with multi-digit counts, whereas the first > just does a bigger circle? Oval is definitely better. I think the second patch > also makes it easier to have a different-colored border as well, if we decide > we want that. Why have one or the other if you can have both! border-radius and min-width/height hackery allows us to create a pretty circle in the single-digit case, but expand to an oval if required. (minimum width is the font size)
The Shell should first ack messages before being able to display the count of unread messages: bug #644297
Created attachment 189583 [details] [review] messageTray: Add an enumerable count for sources to use.
Created attachment 189584 [details] [review] messageTray: Use the count to display the number of unread notifications
Comment on attachment 189584 [details] [review] messageTray: Use the count to display the number of unread notifications I forget why I rejected this, so I'm unrejecting it. Additionally, while I think it would look a bit nicer, the code here doesn't depend on the code in bug 649513. Jon, can you try this out?
Review of attachment 189584 [details] [review]: ::: js/ui/messageTray.js @@ +984,3 @@ this._lastNotificationRemoved(); + + ths.setCount(this.notifications.length); Aha! Now I remember.
Created attachment 190610 [details] [review] messageTray: Use the count to display the number of unread notifications
Created attachment 190611 [details] [review] messageTray: Add an enumerable count for sources to use While testing this patch, I felt that only showing the counter while more than one source was active would be better.
Created attachment 190612 [details] [review] messageTray: Use the count to display the number of unread notifications Fixed the rebase.
Review of attachment 190612 [details] [review]: How can I override this? I tried to display the number of unread messages but can't as this patch override the count I set. See http://cgit.collabora.com/git/user/cassidy/gnome-shell/commit/?h=msg-count&id=2562e8ad3550a096f043a69489a171b2d35cbbf4
Review of attachment 190611 [details] [review]: ::: js/ui/messageTray.js @@ +941,3 @@ + + setCount: function(count) { + let direction = this.actor.get_direction(); I'll need a way to override that. In the chat case, we do want to display "1" if there is one unread message.
Created attachment 190757 [details] [review] messageTray: Add an enumerable count for sources to use
Created attachment 190758 [details] [review] messageTray: Use the count to display the number of unread notifications
Those work fine for me : http://cgit.collabora.com/git/user/cassidy/gnome-shell/commit/?h=msg-count-2&id=02db51ebd6a0534ca9fe980b029e12b02bbdb867
Review of attachment 190758 [details] [review]: ::: js/ui/messageTray.js @@ +946,3 @@ + _updateCount: function() { + let count = this.notificiatons.length; there is a typo here.
Created attachment 190878 [details] [review] messageTray: Use the count to display the number of unread notifications grrr... thanks.
If bug 650392 gets accepted and we always show the summary beside icon, I wonder whether we should put the count on the icon, or just a count in brackets at the end of the summary..
Any chance to get this reviewed and merged? I'd really like to integrate my patch displaying the number of unread messages.
Comment on attachment 190757 [details] [review] messageTray: Add an enumerable count for sources to use >+ let [_, __, naturalWidth, naturalHeight] = this._counterBin.get_preferred_size(); Use real variable names there. This isn't [insert name of language that you picked up that idiom from]. (In particular, "_" is already bound to Gettext.gettext(), so using it for something else is bad and will lead to confusion.) >+ childBox.x1 = this.ICON_SIZE - naturalWidth; >+ childBox.x2 = this.ICON_SIZE; You should allocate iconBin and childBin the same way; either use this.ICON_SIZE for both (ignoring the passed-in box), or use the passed-in box for both (ignoring this.ICON_SIZE). >+ _setCount: function(count, visible) { >+ this._counterBin.visible = visible; >+ this._counterLabel.set_text(count.toString()); Is "visible" really needed? Isn't it just going to be "count != 0"? To prevent abuse by extensions, you might want to check that you can parseInt(count).
Comment on attachment 190878 [details] [review] messageTray: Use the count to display the number of unread notifications >+ _updateCount: function() { >+ let count = this.notifications.length; >+ this._setCount(count, count > 1); if you removed setCount's "visible" param, this would just be this._setCount(count > 1 ? count : 0);
Created attachment 191346 [details] [review] messageTray: Add an enumerable count for sources to use
(In reply to comment #27) > (From update of attachment 190757 [details] [review]) > >+ let [_, __, naturalWidth, naturalHeight] = this._counterBin.get_preferred_size(); > > Use real variable names there. This isn't [insert name of language that you > picked up that idiom from]. (In particular, "_" is already bound to > Gettext.gettext(), so using it for something else is bad and will lead to > confusion.) > > >+ childBox.x1 = this.ICON_SIZE - naturalWidth; > >+ childBox.x2 = this.ICON_SIZE; > > You should allocate iconBin and childBin the same way; either use > this.ICON_SIZE for both (ignoring the passed-in box), or use the passed-in box > for both (ignoring this.ICON_SIZE). I assume you mean counterBin, not childBin. > >+ _setCount: function(count, visible) { > >+ this._counterBin.visible = visible; > >+ this._counterLabel.set_text(count.toString()); > > Is "visible" really needed? Isn't it just going to be "count != 0"? Jon didn't like a "1" counter for notifications (if it's zero, there's no source at all), but wanted one for unread chat messages. > To prevent abuse by extensions, you might want to check that you can > parseInt(count). OK.
Do we want to land this now or wait for the corner-radius bug (bug #649513)?
The patch is marked accepted-commit-now...
What about the other patch?
I meant to imply that it's accepted-commit-now-except-that-you-might-want-to-make-this-change. except you don't, so, commit!
Attachment 190757 [details] pushed as c727da8 - messageTray: Add an enumerable count for sources to use Attachment 190878 [details] pushed as 40f4e92 - messageTray: Use the count to display the number of unread notifications Attachment 191346 [details] pushed as c727da8 - messageTray: Add an enumerable count for sources to use
Created attachment 191605 [details] [review] messageTray: Fix critical in enumerable sources When the bin is hidden, the label gets unrealized, so setting the text fails on a NULL reference. Reopening with a quick fix for: (lt-gnome-shell-real:30467): Clutter-CRITICAL **: clutter_text_get_editable: assertion `CLUTTER_IS_TEXT (self)' failed (lt-gnome-shell-real:30467): Clutter-CRITICAL **: clutter_text_get_text: assertion `CLUTTER_IS_TEXT (self)' failed (lt-gnome-shell-real:30467): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed
Comment on attachment 191605 [details] [review] messageTray: Fix critical in enumerable sources >When the bin is hidden, the label gets unrealized, so setting the text fails >on a NULL reference. No, the problem isn't setting the text when the label is hidden (which is fine), it's setting the text after the source (and hence, the label) has been destroyed.
Florian pushed an alternate fix.