Created attachment 148870 [details] [review]
Add a message tray with icons for ongoing conversations

Add a message tray that slides out when you move your mouse to
the bottom of the screen. The icons for ongoing conversations
are added to the message tray when the first message in the
conversation is received. The icon is removed when the corresponding
conversation window is closed.

Store the avatar icons in the texture cache and use the checksum for
the data bytes for the icon as the key. This allows to reuse the icon
data for the message tray icon.

Add st_box_layout_insert_actor() that allows inserting an actor at the
arbitrary position in the container. It is needed to be able to add the
icon representing the most recent conversation to the front of the list
of icons in the message tray.

Comment on attachment 148870 [details] [review]
Add a message tray with icons for ongoing conversations

So first off, you're not using the names the same way Jon does. According to the spec, both Main.notificationPopup and Main.messageTray are "the message tray". notificationPopup is the message tray in "Notification Mode", and Main.messageTray is the message tray in "Summary Mode". We can worry about that later though.

>+const MESSAGE_TRAY_HEIGHT = 40;

I'm pretty sure you can specify that in the CSS instead

>+    contains: function(id) {
>+        return this._icons.hasOwnProperty(id);
>+    },
>+
>+    add: function(id, icon) {
>+        if (this._icons.hasOwnProperty(id))
>+            return;

should use this.contains() there (and in remove)

>+        let primary = global.get_primary_monitor();
>+        Tweener.addTween(this.actor,
>+                         { y: primary.height - this.actor.height,

>+        let primary = global.get_primary_monitor();
>+
>+        Tweener.addTween(this.actor,
>+                         { y: primary.height - 1,

These will lose if the primary monitor changes, and you don't actually need to use primary.height here anyway; just tween to "this.actor.y + 1 - this.actor.height" on show, and "this.actor.y + this.actor.height - 1" on hide?

>+    hide: function() {
>+        this._isShowing = false;
>+        this._hideTimeoutId = 0;

if this is a public function, then it should deal with the possibility that it's called while _hideTimeoutId is active, and so should explicitly Mainloop.remove_source it. (If it's not a public function then it needs a "_")

>+  checksum = g_compute_checksum_for_data(G_CHECKSUM_SHA1, data, len);

space before (

>+      g_debug("creating new pixbuf");
>+      g_debug("using tecture from cache");

likewise. (and typo on "texture")

>+void
>+st_box_layout_move_child (StBoxLayout  *self,
>+                          ClutterActor *actor,
>+                          int           pos)
>+{
>+  StBoxLayoutPrivate *priv = ST_BOX_LAYOUT (self)->priv;
>+
>+  priv->children = g_list_remove (priv->children, actor);
>+  priv->children = g_list_insert (priv->children, actor, pos);

That only works if you're moving the child to an earlier position; if @pos is greater than @actor's current position then you need to subtract 1 from it after the g_list_remove().


All that said, go ahead and commit this with just the easy fixes and we can fix up other things later and then eventually rebase stuff for the merge to master

after trying it out a little...

the (summary-mode) tray is transparent in the mockups

it feels a little slow to disappear. it might be good to have a series of timeouts, and check the pointer position at each one; if the user moves the mouse far away from the tray, it can probably hide itself immediately. the slow timeout is only needed for the case where the user accidentally moves a bit outside the tray but is about to move back into it

Comment on attachment 148870 [details] [review]
Add a message tray with icons for ongoing conversations

Committed with all the comments applied except for the one about the 'y' parameter in Tweener animation. Setting it based on the actor height and y made the message tray move around if you re-trigger it while it is sliding out, so we'll need to think of a different solution.

Created attachment 149344 [details] [review]
[messaging] update avatar code to use non-deprecated interfaces

I'm attaching some miscellaneous tray-related patches.

This first one removes the use of deprecated telepathy APIs for avatars.

It depends on bug 604023 to work correctly, though IIRC, without that
patch, the only problem is that avatars will initially be wrong (it will use
the default image) but will correct themselves after a minute or so (instead
of starting out correct).

Created attachment 149345 [details] [review]
add an explicit message tray Source type

this starts to merge the "notification mode" and "summary mode" together
a bit more by having Source objects which you add to the summary mode of
the tray, and which you can call "notify" on to have them show up in the
notification mode temporarily as well. The summary mode is updated to
track Source objects explicitly.

Created attachment 149346 [details] [review]
Fix icon/text alignment in message tray

this just fixes the vertical alignment of the text in notifications, to
be centered with the icon

Created attachment 149377 [details] [review]
Implement additional notification daemon icon types

The notification protocol allows for specifying icons in three different
ways: name, path/URI, and raw pixel data. We previously only supported the
first. Of course, the "raw pixel data" is not the same as the telepathy
"raw" image data so we need another new method...

Review of attachment 149344 [details] [review]:

Looks very good overall! This patch leaves non-working code for the icon in the message tray, but a simple patch below fixes it. Other comments are very minor.

diff --git a/js/ui/messaging.js b/js/ui/messaging.js
index a613b8a..7970556 100644
--- a/js/ui/messaging.js
+++ b/js/ui/messaging.js
@@ -371,13 +371,14 @@ Source.prototype = {
         this._channel = new Channel(connName, channelPath);
         this._closedId = this._channel.connect('Closed', Lang.bind(this, this._channelClosed));
 
+        this._targetHandle = targetHandle;
         this._targetId = targetId;
         log('channel for ' + this._targetId + ' channelPath ' + channelPath);
 
         this._pendingMessages = null;
 
-        let connAv = new ConnectionAvatars(conn.getPath());
-        this._avatar = connAv.createAvatar(targetHandle, AVATAR_SIZE);
+        this._connAv = new ConnectionAvatars(conn.getPath());
+        this._avatar = this._connAv.createAvatar(this._targetHandle, AVATAR_SIZE);
 
         this._channelText = new ChannelText(connName, channelPath);
         this._receivedId = this._channelText.connect('Received', Lang.bind(this, this._receivedMessage));
@@ -406,10 +407,7 @@ Source.prototype = {
         Main.notificationPopup.show(this._avatar, text);
         if (!Main.messageTray.contains(this._targetId)) {
             let avatarForMessageTray = null;
-            if (this._avatarBytes)
-                avatarForMessageTray = Shell.TextureCache.get_default().load_from_data(this._avatarBytes, this._avatarBytes.length, AVATAR_SIZE);
-            else
-                avatarForMessageTray = Shell.TextureCache.get_default().load_icon_name("stock_person", AVATAR_SIZE);
+            avatarForMessageTray = this._connAv.createAvatar(this._targetHandle, AVATAR_SIZE);
             Main.messageTray.add(this._targetId, avatarForMessageTray);
         }
     }

::: js/ui/messaging.js
@@ +94,3 @@
+
+        this._avatarData = {};
+        this._icons = {};

Can use a comment on what these maps contain. Something like:

// handle -> avatar byte data for all the handles we ever received the avatars for
this._avatarData = {};
// handle -> array of icon actors currently in use
this._icons = {};

@@ +97,3 @@
+    },
+
+    _AvatarUpdated: function(iface, handle, token) {

I think using _avatarUpdated and _avatarRetrieved is more consistent.

@@ +99,3 @@
+    _AvatarUpdated: function(iface, handle, token) {
+        if (!this._avatarData[handle])
+            return;

When will this happen? We never seem too remove handles from this._avatarData (even though we could when the last icon is destroyed for the handle, but we could also disconnect from 'AvatarUpdated' signal at that time).

@@ +123,3 @@
+            let iconBox = this._icons[handle][i];
+            let size = iconBox.child.height;
+            iconBox.child = this._createIcon(avatarData, size);

This would be a good point to evict from the cache the old data for the icon. We can even use the previous value of this._avatarData[handle] to get the checksum if we don't want to include handles in the cache key.

Can do that in a separate patch though.

@@ +128,3 @@
+
+    createAvatar: function(handle, size) {
+        let iconBox = new St.Bin({ style_class: 'avatar-box' });

Do we need to add 'avatar-box' style in gnome-shell.css ?

@@ +377,1 @@
         this._pendingMessages = null;

this._pendingMessages is no longer used

@@ +379,2 @@
         let connAv = new ConnectionAvatars(conn.getPath());
+        this._avatar = connAv.createAvatar(targetHandle, AVATAR_SIZE);

connAv is not a great name, especially considering avatars is plural in the class name.

(In reply to comment #9)
> Review of attachment 149344 [details] [review]:
> 
> Looks very good overall! This patch leaves non-working code for the icon in the
> message tray

yeah, that actually gets fixed in the next patch. Sorry, these are a little rough, I assume we'll do some rebasing before it all gets merged to master

> +    _AvatarUpdated: function(iface, handle, token) {
> +        if (!this._avatarData[handle])
> +            return;
> 
> When will this happen?

If we get an AvatarUpdated for a handle before receiving the AvatarRetrieved for it. In that case we don't need to call RequestAvatars because we've already requested it. I'll add a comment.

> We never seem too remove handles from this._avatarData
> (even though we could when the last icon is destroyed for the handle, but we
> could also disconnect from 'AvatarUpdated' signal at that time).

Note that Telepathy does absolutely no caching of avatar images itself; when you call RequestAvatars, you're actually going to the IM server (gmail.com, aol.com, whatever) to get the avatar data. So we don't want to call that every time we need to create an icon. The point of _avatarData is to cache the icons of people we're talking to, so we want to keep those icons around until we're not going to need them again.

(The code is a little confusing because Telepathy expects that you're going to cache the avatars to disk between sessions, and so in addition to the contact handles [which are only valid for the lifetime of the connection] it has tokens, which are SHA1sums or something, so that you can tell when you start up again whether or not the avatars are still the same as last time without having to redownload them. But we're not doing that, so we end up having an extra step for no good reason. I'll add a comment about this too.)

We don't clean up _avatarData itself, but it's scoped to the connection object, so when the connection is closed, the cached icon data would be deleted. I guess if you stay logged in forever we'd never delete any icons though. Maybe there should be a max size on this._avatarData...

> +        let iconBox = new St.Bin({ style_class: 'avatar-box' });
> 
> Do we need to add 'avatar-box' style in gnome-shell.css ?

No, it doesn't actually need any styling for what we want, but I figured I'd give it a style_class anyway so that if someone else wants to change things a little they can.

Review of attachment 149345 [details] [review]:

The messages part works well. Is it possible to get a message tray notification for messages that come through the notification daemon? I tried with XChat notifications for channel messages, but they were showing up in the old place.

Good to commit with a few minor fixes.

::: js/ui/messageTray.js
@@ +81,3 @@
 
+function Source(name, icon) {
+    this._init(name, icon);

The parameter names should match the names in _init: function(id, createIcon).

@@ +88,3 @@
+        this.id = id;
+        if (createIcon)
+            this.createIcon = createIcon;

Can probably add a comment that createIcon can be null. I think something like this would be cleaner.

    _init: function(id, createIcon) {
        this.id = id;
        this._createIcon = createIcon;
    },

    /// Virtual public method ///

    createIcon: function() {
        if (this._createIcon)
            return this._createIcon();

        return null;
    },

@@ +92,3 @@
+
+    notify: function(text) {
+        Main.notificationPopup.show(this.createIcon(), text);

This doesn't check if this.createIcon is null, which would fail with the current code if it is null.

@@ +169,3 @@
+        this._tray.remove_actor(this._icons[source.id]);
+        delete this._icons[source.id];
+        delete this._sources[source.id];

We should also remove the notification if there is one associated with this source. We can leave this for a later patch in which we'll queue notifications from different sources and make notification be a mode of the message tray.

::: js/ui/messaging.js
@@ +372,2 @@
     _init: function(conn, channelPath, targetHandle, targetId) {
+        MessageTray.Source.prototype._init.call(this);

Should we at least pass in the id here?

@@ +391,3 @@
     },
 
+    createIcon: function() {

It would be helpful if there was a comment that it is an override.

::: js/ui/notificationDaemon.js
@@ +64,3 @@
     },
 
+    _sourceId: function(id) {

Should be _makeSourceId(); can be static

@@ +70,1 @@
     Notify: function(appName, replacesId, icon, summary, body,

Is it possible to have these function names start with lower case? Or at least make the names of private functions like _NotificationClosed() start with lower case.

Review of attachment 149377 [details] [review]:

Looks like it should work. Some very minor comments.

::: js/ui/messaging.js
@@ +372,2 @@
     _init: function(conn, channelPath, targetHandle, targetId) {
+        MessageTray.Source.prototype._init.call(this, targetId);

Ok, this fixes the problem from the previous commit.

::: js/ui/notificationDaemon.js
@@ +156,3 @@
+        if (this._icon) {
+            if (this._icon.substr(0, 7) == 'file://')
+                return textureCache.load_uri_async(this._icon, 24, 24);

Would be good to replace 24 with a constant. It can be defined in messageTray.js and be common between messaging.js and notificationDaemon.js

@@ +171,3 @@
+            return textureCache.load_icon_name('gtk-dialog-info', 24);
+        }
+    },

Trailing comma.

::: src/shell-texture-cache.c
@@ +1228,3 @@
+ *
+ * Return value: (transfer none): a new #ClutterActor displaying a
+ * pixbuf created from @data, etc.

etc? Probably needs to be more specific or can just end with @data.

Created attachment 150379 [details] [review]
Combine notifications and the summary mode in a single message tray

We should either be showing the message tray in the notification or the summary mode.
This is best achieved if the message tray actor contains both, and either one is shown
at any given time.

Queue notifications so that each queued notification is shown when the previous one
times out or when the user is done interacting with the message tray and moves
the mouse away from it. (In the future, we will have some sort of an indication that
there are queued notifications and a way to have the next notification displayed
without moving the mouse away from the message tray.)

pushed with suggested fixes

Attachment 149344 [details] pushed as 6c3b8e2 - [messaging] update avatar code to use non-deprecated interfaces
Attachment 149345 [details] pushed as ef49ada - add an explicit message tray Source type
Attachment 149346 [details] pushed as 3658f8a - Fix icon/text alignment in message tray
Attachment 149377 [details] pushed as af1a3b1 - Implement additional notification daemon icon types

Created attachment 150844 [details] [review]
Combine notifications and the summary mode in a single message tray

(rebased)

Comment on attachment 150844 [details] [review]
Combine notifications and the summary mode in a single message tray

The "notification" mode doesn't match the most recent mockups I've seen; the full-width gradient should not be visible there, only the centered notification

>+function NotificationContent(icon, text) {

maybe call this just "Notification", and rename the existing "Notification" class to something else?

otherwise it basically looks good

The full-width gradient is visible in the latest mockups as well, it is just more transparent at the top. I made the gradient in the patch more transparent at the top as well.

Renamed Notification->NotificationBox, NotificationContent->Notification.