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 687787 - Don't always show the message tray in the overview, add a new messages indicator
Don't always show the message tray in the overview, add a new messages indicator
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
3.8?
: 694476 (view as bug list)
Depends on:
Blocks: 688530
 
 
Reported: 2012-11-06 17:08 UTC by Allan Day
Modified: 2013-03-04 21:31 UTC
See Also:
GNOME target: 3.8
GNOME version: 3.7/3.8


Attachments
messageTray: emit a signal when we're showing or hiding (2.42 KB, patch)
2013-03-01 02:28 UTC, Cosimo Cecchi
needs-work Details | Review
overview: use a bin for the main overview actor (1.45 KB, patch)
2013-03-01 02:28 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
messageTray: add an indicatorCount property (1.57 KB, patch)
2013-03-01 02:28 UTC, Cosimo Cecchi
reviewed Details | Review
overviewControls: add a new messages indicator (5.92 KB, patch)
2013-03-01 02:29 UTC, Cosimo Cecchi
reviewed Details | Review
overviewControls: add a new messages indicator (6.37 KB, patch)
2013-03-01 02:38 UTC, Cosimo Cecchi
none Details | Review
messageTray: emit a signal when we're showing or hiding (1.13 KB, patch)
2013-03-01 03:14 UTC, Cosimo Cecchi
committed Details | Review
messageTray: add an indicatorCount property (1.52 KB, patch)
2013-03-01 03:21 UTC, Cosimo Cecchi
committed Details | Review
overviewControls: add a new messages indicator (6.42 KB, patch)
2013-03-01 03:22 UTC, Cosimo Cecchi
reviewed Details | Review
overviewControls: add a new messages indicator (6.67 KB, patch)
2013-03-01 04:53 UTC, Cosimo Cecchi
committed Details | Review
screenshot (664.98 KB, image/png)
2013-03-01 15:21 UTC, Cosimo Cecchi
  Details
overview: use a bin for the main overview actor (1.43 KB, patch)
2013-03-01 15:59 UTC, Cosimo Cecchi
committed Details | Review

Description Allan Day 2012-11-06 17:08:22 UTC
Always showing the message tray in the overview has a few disadvantages: it uses a lot of vertical screen space that could be allocated to the window thumbnails, it distracts from application launching and window switching tasks, and it means that we have to use a custom visual style for when the tray is in the overview (leading to inconsistency).

An alternative is to let users manually show and hide the tray as normal, and to add a new messages indicator to the overview.

Mockups for these design updates can be found on the wiki:

https://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/#A3.8_Roadmap

This ties in with bug 687785.
Comment 1 Giovanni Campagna 2013-02-17 18:22:33 UTC
The new messages indicator was rejected in bug 687785, and the visibility of the message tray was fixed in bug 693987.

Closing.
Comment 2 Allan Day 2013-02-17 18:51:20 UTC
I don't think this bug is dependent on bug 687785. We can have a new messages indicator without indicating updated items in the tray.
Comment 3 Giovanni Campagna 2013-02-17 19:12:10 UTC
Uh, you're right, my bad.
Comment 4 Allan Day 2013-02-23 15:13:41 UTC
*** Bug 694476 has been marked as a duplicate of this bug. ***
Comment 5 Cosimo Cecchi 2013-03-01 02:28:12 UTC
Created attachment 237669 [details] [review]
messageTray: emit a signal when we're showing or hiding

This will be useful in a future commit.
Comment 6 Cosimo Cecchi 2013-03-01 02:28:17 UTC
Created attachment 237670 [details] [review]
overview: use a bin for the main overview actor

We'll pack the new messages indicator on top of the current overview
group, so we need to pack it into a ClutterBinLayout first.
Comment 7 Cosimo Cecchi 2013-03-01 02:28:22 UTC
Created attachment 237671 [details] [review]
messageTray: add an indicatorCount property

This filters out resident and transient notifications in the normal
case, but just returns the number of unread messages for the Telepathy
implementation.
Comment 8 Cosimo Cecchi 2013-03-01 02:29:02 UTC
Created attachment 237672 [details] [review]
overviewControls: add a new messages indicator

The new messages indicator will be visible when there are non-resident
notifications in the tray.
Comment 9 Florian Müllner 2013-03-01 02:31:16 UTC
Review of attachment 237670 [details] [review]:

I called it "stack" in bug 694261 :-)
Comment 10 Florian Müllner 2013-03-01 02:31:53 UTC
(In other words, one of us has to rebase patches ...)
Comment 11 Cosimo Cecchi 2013-03-01 02:38:53 UTC
Created attachment 237673 [details] [review]
overviewControls: add a new messages indicator

The new messages indicator will be visible when there are non-resident
notifications in the tray.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-03-01 02:41:03 UTC
Review of attachment 237671 [details] [review]:

I'm not sure I like the name "indicatorCount". I'm also not sure I like that it's not the same number as the sum of all the little blue badges in the tray itself...

::: js/ui/messageTray.js
@@ +1156,3 @@
+    get indicatorCount() {
+        let notifications = this.notifications.filter(
+            function(n) {

Indentation is a bit screwy. You could probably just put the function on the same line.

@@ +1157,3 @@
+        let notifications = this.notifications.filter(
+            function(n) {
+                return (!n.isTransient && !n.resident);

No need for the parens.
Comment 13 Jasper St. Pierre (not reading bugmail) 2013-03-01 02:41:06 UTC
Review of attachment 237672 [details] [review]:

::: js/ui/overviewControls.js
@@ +381,3 @@
+        this.actor.add_actor(this._highlight);
+
+        Main.messageTray.connect('source-added', Lang.bind(this, this._onSourceAdded));

Where do you track existing sources? Shouldn't you need something like:

Main.messageTray.getSources().forEach(Lang.bind(this, function(source) {
    this._onSourceAdded(null, source);
});

?

@@ +398,3 @@
+
+    _onSourceRemoved: function(tray, source) {
+        this._sources = this._sources.filter(function(s) { return s != source });

We usually use this._sources.splice(this._sources.indexOf(source), 1);
Comment 14 Jasper St. Pierre (not reading bugmail) 2013-03-01 02:45:08 UTC
Review of attachment 237669 [details] [review]:

I'd prefer this to be done in updateState, where all the magic is, because otherwise, there are cases where the tray *won't* open, even when traySummoned = true;
Comment 15 Cosimo Cecchi 2013-03-01 02:46:57 UTC
(In reply to comment #13)

> Where do you track existing sources? Shouldn't you need something like:

Well ControlsManager is instantiated only once at startup, so I don't think that's really needed - I believe the screen shield code needs it because it's recreated every time the screen is locked.
Comment 16 Jasper St. Pierre (not reading bugmail) 2013-03-01 02:55:18 UTC
Ah, I thought ControlsManager was recreated with the rest of the overview.
Comment 17 Cosimo Cecchi 2013-03-01 03:14:53 UTC
Created attachment 237675 [details] [review]
messageTray: emit a signal when we're showing or hiding

--

Okay; showTray and hideTray are only called from updateState so it should be fine to emit the signal just before the tween starts.
Comment 18 Cosimo Cecchi 2013-03-01 03:21:52 UTC
Created attachment 237677 [details] [review]
messageTray: add an indicatorCount property

--

They are some subtle differences for resident notification handling with the badge count, which represents the total notifications for the source; e.g. an app that has a resident notification and then adds another regular one has a (2) badge but only one new message.

The alternative is trying to filter out resident notifications inside the indicator, but I didn't like it, since Telepathy notifications are also resident but behave differently and would need special casing.
Comment 19 Cosimo Cecchi 2013-03-01 03:22:40 UTC
Created attachment 237678 [details] [review]
overviewControls: add a new messages indicator

The new messages indicator will be visible when there are non-resident
notifications in the tray.
Comment 20 Cosimo Cecchi 2013-03-01 03:41:36 UTC
(In reply to comment #10)
> (In other words, one of us has to rebase patches ...)

Maybe I can just rename it to stack and we get the small patch in first?
Comment 21 Florian Müllner 2013-03-01 03:46:32 UTC
(In reply to comment #20)
> (In reply to comment #10)
> > (In other words, one of us has to rebase patches ...)
> 
> Maybe I can just rename it to stack and we get the small patch in first?

I should be able to manage a s/stack/overviewBin/ as well :-)

(there's a small problem with StBin and ClutterBinLayout being very different, so the name "bin" is a bit confusing)
Comment 22 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:16:53 UTC
Review of attachment 237670 [details] [review]:

I like the name "stack" better.
Comment 23 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:17:16 UTC
Review of attachment 237675 [details] [review]:

OK.
Comment 24 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:17:38 UTC
Review of attachment 237677 [details] [review]:

If this is the design, go for it, I guess.
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:29:09 UTC
Review of attachment 237678 [details] [review]:

::: data/theme/gnome-shell.css
@@ +759,3 @@
+.messages-indicator-highlight {
+    background-gradient-start: transparent;
+    background-gradient-end: #999999;

If we wanted to rip Android off, we could use their color scheme of using #33B5E5 for this thing:

http://developer.android.com/design/style/color.html

::: js/ui/overviewControls.js
@@ +385,3 @@
+        this.actor.add_actor(this._highlight);
+
+        Main.messageTray.connect('source-added', Lang.bind(this, this._onSourceAdded));

Either add a comment about how we're expected to be initialized during startup, and thus we can't possibly have any sources, or just do the getSources iteration and append sources. I'm leaning towards the latter, as it's cleaner and slightly more extension friendly.

@@ +416,3 @@
+
+        this._count = count;
+        this._label.text = _("%d new messages").format(count);

Shouldn't this use ngettext? Otherwise we'll have "1 new messages"
Comment 26 Cosimo Cecchi 2013-03-01 04:53:55 UTC
Created attachment 237683 [details] [review]
overviewControls: add a new messages indicator

--

Fixed to use ngettext, and also adding sources at creation.
As for the style, I think it's best to leave that for Jakub or Allan to tweak - right now it's using a simple light gray to transparent gradient.
Comment 27 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:56:10 UTC
Review of attachment 237683 [details] [review]:

OK.
Comment 28 Cosimo Cecchi 2013-03-01 14:25:19 UTC
I asked for an UI freeze break for this now.
Comment 29 Cosimo Cecchi 2013-03-01 15:21:36 UTC
Created attachment 237717 [details]
screenshot
Comment 30 Florian Müllner 2013-03-01 15:23:12 UTC
(In reply to comment #29)
> Created an attachment (id=237717) [details]
> screenshot

Off-topic: looking at your terminal, I feel less alone now ...
Comment 31 Cosimo Cecchi 2013-03-01 15:59:54 UTC
Created attachment 237718 [details] [review]
overview: use a bin for the main overview actor

--

Reattaching the patch using _stack so people can test.
Comment 32 Cosimo Cecchi 2013-03-01 16:00:13 UTC
Comment on attachment 237718 [details] [review]
overview: use a bin for the main overview actor

Restoring patch status
Comment 33 Cosimo Cecchi 2013-03-01 16:35:21 UTC
Comment on attachment 237718 [details] [review]
overview: use a bin for the main overview actor

Attachment 237718 [details] pushed as 43ed66c - overview: use a bin for the main overview actor

Getting this out of the way so Florian can rebase his patchset for bug 694261
Comment 34 Cosimo Cecchi 2013-03-01 18:51:47 UTC
Attachment 237675 [details] pushed as 8fb2263 - messageTray: emit a signal when we're showing or hiding
Attachment 237677 [details] pushed as bdbea24 - messageTray: add an indicatorCount property
Attachment 237683 [details] pushed as 8b48560 - overviewControls: add a new messages indicator
Comment 35 Allan Day 2013-03-04 09:08:29 UTC
Thanks Cosimo, this looks great. One thing: the chat icon was intended to indicate if there are chat notifications waiting - it wasn't supposed to be a permanent presence.
Comment 36 Cosimo Cecchi 2013-03-04 21:31:37 UTC
That is now fixed too in git master.