After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 649356 - Implement the "Starburst" in the message tray.
Implement the "Starburst" in the message tray.
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 653478
Blocks:
 
 
Reported: 2011-05-04 08:29 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2011-07-13 18:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
messageTray: first hack at a starburst count (6.86 KB, patch)
2011-05-04 08:29 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: second hack at a starburst count (4.71 KB, patch)
2011-05-06 03:25 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Add an enumerable count for sources to use. (4.02 KB, patch)
2011-06-05 08:26 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
notificationDaemon: Add a new notification hint 'count' (1.47 KB, patch)
2011-06-05 08:27 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
messageTray: Add an enumerable count for sources to use. (3.92 KB, patch)
2011-06-09 21:23 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Use the count to display the number of unread notifications (1.25 KB, patch)
2011-06-09 21:24 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
messageTray: Use the count to display the number of unread notifications (1.49 KB, patch)
2011-06-24 19:42 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Add an enumerable count for sources to use (3.92 KB, patch)
2011-06-24 19:48 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Use the count to display the number of unread notifications (1.25 KB, patch)
2011-06-24 19:48 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Add an enumerable count for sources to use (3.93 KB, patch)
2011-06-27 14:37 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Use the count to display the number of unread notifications (1.52 KB, patch)
2011-06-27 14:37 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Use the count to display the number of unread notifications (1.52 KB, patch)
2011-06-28 15:30 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Add an enumerable count for sources to use (4.01 KB, patch)
2011-07-05 18:00 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Fix critical in enumerable sources (949 bytes, patch)
2011-07-09 23:56 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review

Description Jasper St. Pierre (not reading bugmail) 2011-05-04 08:29:54 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.
Comment 1 Jasper St. Pierre (not reading bugmail) 2011-05-04 08:29:58 UTC
Created attachment 187172 [details] [review]
messageTray: first hack at a starburst count
Comment 2 Jasper St. Pierre (not reading bugmail) 2011-05-04 08:34:38 UTC
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.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-05-06 03:25:55 UTC
Created attachment 187336 [details] [review]
messageTray: second hack at a starburst count
Comment 4 Jasper St. Pierre (not reading bugmail) 2011-05-06 03:27:03 UTC
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
Comment 5 Jonny Lamb 2011-05-24 12:31:23 UTC
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.
Comment 6 Dan Winship 2011-05-24 15:13:32 UTC
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.
Comment 7 Jasper St. Pierre (not reading bugmail) 2011-06-05 08:26:41 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-06-05 08:27:22 UTC
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".
Comment 9 Jasper St. Pierre (not reading bugmail) 2011-06-05 08:35:11 UTC
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)
Comment 10 Guillaume Desmottes 2011-06-09 12:19:46 UTC
The Shell should first ack messages before being able to display the count of unread messages: bug #644297
Comment 11 Jasper St. Pierre (not reading bugmail) 2011-06-09 21:23:51 UTC
Created attachment 189583 [details] [review]
messageTray: Add an enumerable count for sources to use.
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-06-09 21:24:01 UTC
Created attachment 189584 [details] [review]
messageTray: Use the count to display the number of unread notifications
Comment 13 Jasper St. Pierre (not reading bugmail) 2011-06-24 17:53:47 UTC
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?
Comment 14 Jasper St. Pierre (not reading bugmail) 2011-06-24 19:42:04 UTC
Review of attachment 189584 [details] [review]:

::: js/ui/messageTray.js
@@ +984,3 @@
                     this._lastNotificationRemoved();
+
+                ths.setCount(this.notifications.length);

Aha! Now I remember.
Comment 15 Jasper St. Pierre (not reading bugmail) 2011-06-24 19:42:24 UTC
Created attachment 190610 [details] [review]
messageTray: Use the count to display the number of unread notifications
Comment 16 Jasper St. Pierre (not reading bugmail) 2011-06-24 19:48:11 UTC
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.
Comment 17 Jasper St. Pierre (not reading bugmail) 2011-06-24 19:48:43 UTC
Created attachment 190612 [details] [review]
messageTray: Use the count to display the number of unread notifications

Fixed the rebase.
Comment 18 Guillaume Desmottes 2011-06-27 12:33:19 UTC
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
Comment 19 Guillaume Desmottes 2011-06-27 12:40:29 UTC
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.
Comment 20 Jasper St. Pierre (not reading bugmail) 2011-06-27 14:37:26 UTC
Created attachment 190757 [details] [review]
messageTray: Add an enumerable count for sources to use
Comment 21 Jasper St. Pierre (not reading bugmail) 2011-06-27 14:37:33 UTC
Created attachment 190758 [details] [review]
messageTray: Use the count to display the number of unread notifications
Comment 23 Guillaume Desmottes 2011-06-28 08:37:13 UTC
Review of attachment 190758 [details] [review]:

::: js/ui/messageTray.js
@@ +946,3 @@
 
+    _updateCount: function() {
+        let count = this.notificiatons.length;

there is a typo here.
Comment 24 Jasper St. Pierre (not reading bugmail) 2011-06-28 15:30:50 UTC
Created attachment 190878 [details] [review]
messageTray: Use the count to display the number of unread notifications

grrr... thanks.
Comment 25 Nguyen Thai Ngoc Duy 2011-06-29 02:23:00 UTC
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..
Comment 26 Guillaume Desmottes 2011-07-04 14:06:07 UTC
Any chance to get this reviewed and merged? I'd really like to integrate my patch displaying the number of unread messages.
Comment 27 Dan Winship 2011-07-05 12:24:23 UTC
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 28 Dan Winship 2011-07-05 12:28:34 UTC
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);
Comment 29 Jasper St. Pierre (not reading bugmail) 2011-07-05 18:00:07 UTC
Created attachment 191346 [details] [review]
messageTray: Add an enumerable count for sources to use
Comment 30 Jasper St. Pierre (not reading bugmail) 2011-07-05 18:00:17 UTC
(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.
Comment 31 Jasper St. Pierre (not reading bugmail) 2011-07-05 22:00:39 UTC
Do we want to land this now or wait for the corner-radius bug (bug #649513)?
Comment 32 Matthias Clasen 2011-07-06 11:36:03 UTC
The patch is marked accepted-commit-now...
Comment 33 Jasper St. Pierre (not reading bugmail) 2011-07-06 16:49:33 UTC
What about the other patch?
Comment 34 Dan Winship 2011-07-06 22:17:55 UTC
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!
Comment 35 Jasper St. Pierre (not reading bugmail) 2011-07-06 22:45:12 UTC
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
Comment 36 Jasper St. Pierre (not reading bugmail) 2011-07-09 23:56:27 UTC
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 37 Dan Winship 2011-07-11 15:32:21 UTC
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.
Comment 38 Jasper St. Pierre (not reading bugmail) 2011-07-13 18:49:29 UTC
Florian pushed an alternate fix.