We should get the existing per-app source fixes in first (which is part of bug 607375)

It seems wrong to add a second notification tray in the message tray. If we are going to move trayicons down there, the right way would be to simply have the icons show up as sources in the summary area, mixed in with all the other sources. And then since libnotify lets an app indicate that a notification is associated with a status icon (via the window-xid hint), we'll know to use the existing trayicon-based Source for those notifications rather than creating a new generic Source like for an ordinary notification.

Comment on attachment 152910 [details] [review]
Split the TrayManager icons into two trays

Marking needs work based on comment 2

Created attachment 165997 [details] [review]
Move non system tray icons to message tray

The modifications to src/gnome-shell-plugin.c make hover tracking with the XEmbed status icon windows work (credit goes to owen).

StatusIconDispatcher sends the icons on the magic system tray list, and packages the ones for the message tray in sources that inherit from NotificationDaemon.Source so they play well with notifications.

Review of attachment 165997 [details] [review]:

::: js/ui/messageTray.js
@@ +13,2 @@
 const Main = imports.ui.main;
+const NotificationDaemon = imports.ui.notificationDaemon;

messageTray.js should not need to import NotificationDaemon. (and afaict, you don't use it)

@@ +593,3 @@
+    update: function(icon, hints) {
+        this._icon = icon;
+        this._iconData = hints.icon_data;

this is wrong. "hints" and "hints.icon_data" are aspects of how NotificationDaemon works, and should not be exposed to messageTray.js

::: js/ui/notificationDaemon.js
@@ +144,2 @@
     _sourceId: function(id) {
+        return 'source-' + id.toLowerCase();

why toLowerCase?

@@ +211,3 @@
             notification.connect('dismissed', Lang.bind(this,
                 function(n) {
+                    n.destroy();

this change is already in git; you need to rebase

@@ +368,3 @@
+    },
+
+    notify: function(notification) {

This appears to jus be an exact copy of MessageTray.Source.prototype.notify, which you already inherit. You shouldn't need this at all.

@@ +385,3 @@
     }
 };
+Signals.addSignalMethods(Source.prototype);

likewise, this shouldn't be necessary; it already gets the signal methods from MessageTray.Source.prototype.

(this means you don't need the Signals import at the top either)

::: js/ui/panel.js
@@ +849,3 @@
 
+        // Figure out the index in our well-known order for this icon
+        let position = STANDARD_TRAY_ICON_ORDER.indexOf(icon._role);

"_"-prefixed names are private. If you need to be looking at icon._role from here, then it's not private, and it should be called "icon.role" instead.

@@ +877,3 @@
+        if (this._trayBox.get_children().length == 0)
+            this._trayBox.hide();
+        this._recomputeTraySize();

as noted in the review of the original patch, we can get rid of the whole standard-vs-compact spacing thing (although that can be a separate patch)

::: js/ui/statusIconDispatcher.js
@@ +16,3 @@
+const Place = {
+    MESSAGE_TRAY: 0,
+    STATUS_TRAY: 1

call it STATUS_AREA, not STATUS_TRAY, please

@@ +48,3 @@
+	    this._icons[icon] = Place.STATUS_TRAY;
+	    icon._role = role;
+	    this.emit('status-icon-added', icon, wmClass);

it's a little ugly that you have an addMessageTrayIcon() method, but for status icons you just emit the signal directly...

ah, and I see why you're using ._role rather than .role here; because the icon object is actually a wrapper of a ClutterActor...

well, actually, you don't need to set the role on the icon at all; just emit "icon, wmClass, role" instead of "icon, wmClass".

@@ +71,3 @@
+            return;
+
+        let source = new Source(wmClass, wmClassMap[wmClass] || wmClass, icon);

ugh. ok, again, this creates messy interconnections between the different classes.

We don't need to map WM_CLASS values to match notificationDaemon clients to trayicons. The application will call notify_notification_attach_to_status_icon() on the notification, which will cause libnotify to add a "window-xid" hint to the notification pointing to the status icon's plug window (the "icon_window" variable in na_tray_manager_handle_dock_request(), which we could either include as part of the tray-icon-added signal, or else make accessible as a property on the NaTrayChild object).

So when a tray icon is created, we'd want to create a source identified with that window, and then later when notificationDaemon gets a notification with a corresponding window-xid, it would find that source and use it rather than looking for/creating a source based on the appName.

Related to all of this: the way source ids are currently handled is not very logical; the message tray keeps track of them even though it doesn't need to, and then notificationDaemon calls into a messageTray method to find its own sources. MessageTray doesn't need IDs at all; it can just do comparisons by source objects directly, and then notificationDaemon could just store the list of active (libnotify-based and trayicon-based) sources itself, indexed however it finds convenient (sometimes by appName, sometimes by window-xid).

@@ +102,3 @@
+
+        this.createIcon = NotificationDaemon.Source.prototype.createIcon;
+        return this._window;

so this returns the embedded window on the first call and then an icon on successive calls? No, that makes too many assumptions about how MessageTray works. We need to split this into separate getSummaryIcon() and createNotificationIcon() methods.

@@ +105,3 @@
+    }
+};
+Signals.addSignalMethods(Source.prototype);

not needed (and likewise the import)

::: src/gnome-shell-plugin.c
@@ +507,3 @@
+      && xev->xcrossing.detail == NotifyInferior
+      && xev->xcrossing.window == clutter_x11_get_stage_window (CLUTTER_STAGE (clutter_stage_get_default ())))
+    return TRUE;

why?

(In reply to comment #4)
> @@ +593,3 @@
> +    update: function(icon, hints) {
> +        this._icon = icon;
> +        this._iconData = hints.icon_data;
> 
> this is wrong. "hints" and "hints.icon_data" are aspects of how
> NotificationDaemon works, and should not be exposed to messageTray.js
That function already takes those arguments, all I did was changing things so we store more info and it's not locked to the icon_data hint.

> ::: js/ui/notificationDaemon.js
> @@ +144,2 @@
>      _sourceId: function(id) {
> +        return 'source-' + id.toLowerCase();
> 
> why toLowerCase?
To match programs if their wmClass and whatever name they pass to libnotify have different case.  Admittedly a hack, the window id would be better, but I wanted to get the patch up for comments and hadn't yet figured that detail out.

> @@ +368,3 @@
> +    },
> +
> +    notify: function(notification) {
> 
> This appears to jus be an exact copy of MessageTray.Source.prototype.notify,
> which you already inherit. You shouldn't need this at all.
> 
> @@ +385,3 @@
>      }
>  };
> +Signals.addSignalMethods(Source.prototype);
> 
> likewise, this shouldn't be necessary; it already gets the signal methods from
> MessageTray.Source.prototype.
> 
> (this means you don't need the Signals import at the top either)
I was getting errors about not being able to call .connect() on the source until I added these.


> ::: src/gnome-shell-plugin.c
> @@ +507,3 @@
> +      && xev->xcrossing.detail == NotifyInferior
> +      && xev->xcrossing.window == clutter_x11_get_stage_window (CLUTTER_STAGE
> (clutter_stage_get_default ())))
> +    return TRUE;
> 
> why?

As mentioned above
(In reply to comment #3)
> The modifications to src/gnome-shell-plugin.c make hover tracking with the
> XEmbed status icon windows work (credit goes to owen).

That was so the tray doesn't hide itself when you move the mouse over an icon (which is an embedded window).

(In reply to comment #5)
> (In reply to comment #4)
> > @@ +593,3 @@
> > +    update: function(icon, hints) {
> > +        this._icon = icon;
> > +        this._iconData = hints.icon_data;
> > 
> > this is wrong. "hints" and "hints.icon_data" are aspects of how
> > NotificationDaemon works, and should not be exposed to messageTray.js
> That function already takes those arguments, all I did was changing things so
> we store more info and it's not locked to the icon_data hint.

No, there's a function NotificationDaemon.Source.update() which takes icon and hints, and that's fine. But you added (roughly) the same method to MessageTray.Source, which is bad, because "hints" is basically a private type for NotificationDaemon.

And actually... as far as I can tell, nothing in your patch makes use of the MessageTray.Source.update(), because you don't remove NotificationDaemon.Source.update(), and you don't add any new calls to update() anywhere else, so all the things that used to be using the NotificationDaemon version still are.

> I was getting errors about not being able to call .connect() on the source
> until I added these.

Did you change anything else later on? If you weren't initializing __proto__ correctly on StatusIconDispatcher.Source.prototype, you'd get this problem, but you shouldn't otherwise. Does it work correctly if you remove those pieces now?

Created attachment 166648 [details] [review]
Move non system tray icons to message tray

Hopefully I've removed all of the incorrect corss-over between modules and cleaned things up in general.  There are still definite issues posed by the status icons in the tray including:

- Hovering over status icons doesn't activate the expand effect
- Sometimes clicking on bits of a SummaryItem's title triggers and icon's actions (summoning window, right click menu)
- I took out the attempts to associate notifications with their application pending a proper implementation taking advantage of the window-xid hint.

Fixing the remaining bits is going to require some X ickiness.

(In reply to comment #7)
> - Hovering over status icons doesn't activate the expand effect

Right. So the problem is that we don't currently get events on the embedded icons. We need to select for crossing events on those windows, and then tweak ShellGtkEmbed to consider the pointer to still be inside it when it's actually inside the embedded X window. Possibly involving blocking/faking the emission of enter/leave events? Or something?

> - Sometimes clicking on bits of a SummaryItem's title triggers and icon's
> actions (summoning window, right click menu)

The problem here is that ShellGtkEmbed assumes its parent will use allocation, not fixed position, to position it, and it only updates the position of the X window when it's reallocated. This needs to be updated to move the X window when the actor is re-positioned as well.

Created attachment 168237 [details] [review]
[ShellGtkEmbed] do a better job of tracking the actor's position

Previously the code to keep the X window aligned with its actor made
various assumptions that were true in the panel status tray area but
that are not true in the message tray. Fix it up by repositioning the
X window at clutter_actor_paint() time, which will definitely be
called every time the actor moves, whether it's because of its own or
its parent/ancestor's changes.

Created attachment 168238 [details] [review]
[ShellGtkEmbed] add a debugging mode

The debugging mode adds visible frames around the current locations of
embedded windows, so you can verify that they are properly tracking
their corresponding ClutterActors.

Created attachment 170269 [details] [review]
[ShellGtkEmbed] do a better job of tracking the actor's position

Previously the code to keep the X window aligned with its actor made
various assumptions that were true in the panel status tray area but
that are not true in the message tray. Fix it up by repositioning the
X window at clutter_actor_paint() time, which will definitely be
called every time the actor moves, whether it's because of its own or
its parent/ancestor's changes.

Created attachment 170270 [details] [review]
[ShellGtkEmbed] add a debugging mode

The debugging mode adds visible frames around the current locations of
embedded windows, so you can verify that they are properly tracking
their corresponding ClutterActors.

Created attachment 170272 [details] [review]
[tray] add ShellTrayIcon, make ShellTrayManager use it

The actor emitted by ShellTrayManager is now ShellTrayIcon, which has
several properties on it which are (or will soon be) useful to the
shell.

Ideally, ShellTrayIcon would have been a subclass of ShellGtkEmbed,
but this is not easily possible given that ShellGtkEmbed's struct is a
private type. If XEMBED-based trayicons are going to around in the
long term, we ought to clean up ShellGtkEmbed/ShellEmbeddedWindow a
bunch, but it's not worth it if they're going to just end up being
deprecated.

Created attachment 170273 [details] [review]
[MessageTray] show newly-added sources even if they don't post notifications

eg, when a trayicon is added, show the summary briefly

Created attachment 170274 [details] [review]
[StatusIconDispatcher] Move non-system-tray icons to message tray

New StatusIconDispatcher dispatches icons to the system or message tray.

Based on a patch from Matt Novenstern

this is ready for review now.

one way to test the combined-trayicon-and-notification behavior is to "yum downgrade" something, which should then cause the packagekit icon to appear and pop up a notification telling you updates are available

Review of attachment 170269 [details] [review]:

Won't say I *like* this patch, but I think it's OK to commit, with some comment fixes mentioned below.

::: src/shell-gtk-embed.c
@@ +202,3 @@
+
+  clutter_actor_get_transformed_position (actor, &x, &y);
+  clutter_actor_get_transformed_size (actor, &w, &h);

This actually involves hundreds floating point operations for each of these - it's not nearly as innocent as it looks. Calling clutter_actor_get_abs_allocation_vertices() and deciphering the position and size out of that would be half the cost, but a lot more code.

It's not really something we want to be doing for every paint. Nor honestly, is adding an idle. (The main loop breaks down when too many sources are added - it's a big linked list.)

In the end, this is unlikely to be measurable for a few tray icons. And I don't have good alternate suggestions. So I'm just grumpy about it :-)

@@ +222,3 @@
+   * position and allocation of @embed and each of its ancestors as
+   * they change. However, we can't actually do it directly from
+   * here, because get_transformed_size() will fail.

A bit confused "it's easier to just do queue this from paint() .... However we can't do it directly." - first 'queue' is probably extraneous.

Completely unclear to me why get_transformed_size() would fail from paint. Doesn't make any sense, actually. So, I think it needs an explanation or a bugzilla.clutter-project.org link.

Review of attachment 170270 [details] [review]:

Don't like having a debug mode change the actor hierarchy. Why can't you do this out of paint() and just paint the box there? Cogl isn't a great drawing API but it's easy enough to use for simple stuff like rectangle. Should be much less code. (Also, I think you need docs for set_debug() since the name is undescriptive.)

Review of attachment 170272 [details] [review]:

Generally looks fine and like an improvement in encapsulation

XEMBED icons are clearly not going away any time in the next couple of years, since they are created by many applications outside of our control. We'll certainly have code around handling them for a while. I don't have a strong opinion that it's wrong for ShelTrayIcon to have-a ShellGtkEmbed ... but it does seem like making ShellGtkEmbed derivable would be a smaller change then some of the other stuff. You could just move the structure into the public headers and skip the Priv stuff. (I personally don't see any point in using private structures in something that never will be part of a public library, but I guess other people feel that it's easier to just always write GObjects one way.)

::: src/shell-embedded-window.c
@@ +42,3 @@
+   PROP_0,
+
+   PROP_STAGE

I don't absolutely object to passing in the stage instead of calling clutter_stage_get_default(), but I'm confused as to what this has to do with the patch. Presumably we don't actually have multiple stages in the process?

::: src/shell-tray-icon.c
@@ +89,3 @@
+        g_object_unref (icon->priv->embed);
+      icon->priv->embed = shell_gtk_embed_new (window);
+      st_bin_set_child (ST_BIN (icon), icon->priv->embed);

This looks like an adopted flaoting reference count here, but you unref in dispose()

@@ +151,3 @@
+    {
+    case PROP_APP:
+      if (!icon->priv->app && icon->priv->pid)

Doesn't this have the same issue as:

112	 /* We do all this now rather than computing it on the fly later,
113	* because the shell may want to see their values from a
114	* tray-icon-removed signal handler, at which point the plug has
115	* already been removed from the socket.

? If it's too expensive to do that for the app, should be commented that you made that decision.

@@ +218,3 @@
+                                                        "PID",
+                                                        "The PID of the icon's application",
+                                                        0, G_MAXUINT64, 0,

Might want to briefly comment why you use a 64-bit here - presumably this isn't unmotivated future proofing?

::: src/shell-tray-manager.c
@@ -288,3 @@
-   * get the first plugged notification is to be able to get the WM_CLASS
-   * from the child window.  But we don't want to emit this signal twice
-   * if for some reason the socket gets replugged.

Did this have a point? If it had a point, was it replaced in your patch in some way?

Review of attachment 170273 [details] [review]:

::: js/ui/messageTray.js
@@ +991,3 @@
+        // *first* and not show the summary item until after it hides.
+        // So postpone calling _updateState() a tiny bit.
+        Mainloop.idle_add(Lang.bind(this, function() { this._updateState(); return false; }));

Mainlop.idle_add() is not deterministically called; if the shell, is for example, continuously redrawing because an animation is in progress or glxgears is running, it won't be called. I think it's basically blanket wrong. Meta.later_add() should generally be used with the appropriate constant.

I don't really have the background to review this in the context of the message tray, seems reasonable.

Review of attachment 170274 [details] [review]:

Though I'm not really familiar the message tray code, this patch looks good to me except for a few things noted below.

::: js/ui/notificationDaemon.js
@@ +178,3 @@
+                source.destroy();
+            }));
+        source.connect('destroy', Lang.bind(this,

Maybe it makes sense to have some function to insert a source that does the _sources[key] and the connection to remove the key, since you have that in three places? Or does it make sense to have this as

_newSource: function(app, title, keys) 

?

@@ +238,3 @@
 
+        if (!source && hints['window-xid'])
+            source = this._sources[hints['window-xid']];

Hmmm, this is to save a D-Bus round-trip?

@@ +384,3 @@
+
+        if (!this._sources[icon.plug_xid]) {
+            this._sources[icon.plug_xid] = source;

I don't think it's OK to mix XID's and PID's in the same array without distinguishing them. Maybe you never get a collision, but when you do, boy will you be confused. And it's rather confusing to read as well.

It also took me a while to find why you needed the XID lookup, so maybe comment that?

::: js/ui/statusIconDispatcher.js
@@ +61,3 @@
+        for (let i = 0; i < this._icons.length; i++) {
+            let icon = this._icons[i].icon;
+            if (icon instanceof Shell.TrayIcon)

There doesn't seem to be any this._icons member

::: src/gnome-shell-plugin.c
@@ +506,3 @@
 #endif
 
+  if ((xev->xany.type == EnterNotify || xev->xany.type == LeaveNotify)

This magic workaround needs a comment. (I can explain it if you need it)

(In reply to comment #17)
> +  clutter_actor_get_transformed_position (actor, &x, &y);
> +  clutter_actor_get_transformed_size (actor, &w, &h);
> 
> This actually involves hundreds floating point operations for each of these -
> it's not nearly as innocent as it looks.

If we assume that the chrome will never be scaled or rotated (which seems reasonable), we could just walk up the widget hierarchy and add up the coordinates, which I assume would be much less work?

> Completely unclear to me why get_transformed_size() would fail from paint.

Changing the code, it seems to work now. I must have had some other bug in the code when I was testing that...

(In reply to comment #19)
> I don't have a strong
> opinion that it's wrong for ShelTrayIcon to have-a ShellGtkEmbed ... but it
> does seem like making ShellGtkEmbed derivable would be a smaller change then
> some of the other stuff. You could just move the structure into the public
> headers and skip the Priv stuff.

The problem is that a ShellGtkEmbed is-a ClutterGLXTexturePixmap and ClutterGLX is not introspected.

> I don't absolutely object to passing in the stage instead of calling
> clutter_stage_get_default(), but I'm confused as to what this has to do with
> the patch. Presumably we don't actually have multiple stages in the process?

Previously it was calling clutter_actor_get_stage() on the ShellGtkEmbed, but with the reorganization, that code will get called before the ShellEmbeddedWindow has a ShellGtkEmbed assigned to it.

I avoided using clutter_stage_get_default() because the existing code (usually) did. But I'm fine getting rid of the passed-in stage.

> +    case PROP_APP:
> +      if (!icon->priv->app && icon->priv->pid)
> 
> Doesn't this have the same issue as:
> 
> 112     /* We do all this now rather than computing it on the fly later,
> 113    * because the shell may want to see their values from a
> 114    * tray-icon-removed signal handler, at which point the plug has
> 115    * already been removed from the socket.

No, because we already extracted icon->priv->pid from the embedded window, so we don't need to have the window still around to be able to try to look up its app. The reason we don't pre-cache the ShellApp is that ShellApps are only created for processes that create toplevel windows, so if it creates its trayicon before its toplevel window, then it won't have a ShellApp at construct time.

However, I think in that case the ShellApp would never get assigned to the NotificationDaemon.Source, which is a bug.

> Might want to briefly comment why you use a 64-bit here - presumably this isn't
> unmotivated future proofing?

thinko, as discussed on irc

> -   * get the first plugged notification is to be able to get the WM_CLASS
> -   * from the child window.  But we don't want to emit this signal twice
> -   * if for some reason the socket gets replugged.
> 
> Did this have a point? If it had a point, was it replaced in your patch in some
> way?

we disconnect from the signal now after its emitted, rather than just ignoring future emissions.

(In reply to comment #22)
> (In reply to comment #17)
> > +  clutter_actor_get_transformed_position (actor, &x, &y);
> > +  clutter_actor_get_transformed_size (actor, &w, &h);
> > 
> > This actually involves hundreds floating point operations for each of these -
> > it's not nearly as innocent as it looks.
> 
> If we assume that the chrome will never be scaled or rotated (which seems
> reasonable), we could just walk up the widget hierarchy and add up the
> coordinates, which I assume would be much less work?

It is (at least in terms of not computing the full transformation matrix twice, etc.). I'm not sure if that's why I was doing it that way in the old code, or if I had some other better or worse reason.

(In reply to comment #23)
> (In reply to comment #19)
> > I don't have a strong
> > opinion that it's wrong for ShelTrayIcon to have-a ShellGtkEmbed ... but it
> > does seem like making ShellGtkEmbed derivable would be a smaller change then
> > some of the other stuff. You could just move the structure into the public
> > headers and skip the Priv stuff.
> 
> The problem is that a ShellGtkEmbed is-a ClutterGLXTexturePixmap and 
> ClutterGLX is not introspected.

Discussed on IRC - the problem is that having ShellGtkEmbed structure in a public header would upset introspection since it doesn't know sizeof(ClutterGLXTexturePixmap). But having it in a -private.h that isn't scanned should be fine.

> > I don't absolutely object to passing in the stage instead of calling
> > clutter_stage_get_default(), but I'm confused as to what this has to do with
> > the patch. Presumably we don't actually have multiple stages in the process?
> 
> Previously it was calling clutter_actor_get_stage() on the ShellGtkEmbed, but
> with the reorganization, that code will get called before the
> ShellEmbeddedWindow has a ShellGtkEmbed assigned to it.
> 
> I avoided using clutter_stage_get_default() because the existing code 
> (usually) did. But I'm fine getting rid of the passed-in stage.

I don't really care. Just wondering if there was a particular reason you were jumping through hoops. :-)
 
> > +    case PROP_APP:
> > +      if (!icon->priv->app && icon->priv->pid)
> > 
> > Doesn't this have the same issue as:
> > 
> > 112     /* We do all this now rather than computing it on the fly later,
> > 113    * because the shell may want to see their values from a
> > 114    * tray-icon-removed signal handler, at which point the plug has
> > 115    * already been removed from the socket.
> 
> No, because we already extracted icon->priv->pid from the embedded window, so
> we don't need to have the window still around to be able to try to look up its
> app. The reason we don't pre-cache the ShellApp is that ShellApps are only
> created for processes that create toplevel windows, so if it creates its
> trayicon before its toplevel window, then it won't have a ShellApp at 
> construct time.

OK. But at remove time the Shell.App might be gone too? Maybe a remark about the app would be useful in the comment where you explain looking up to xid would be useful - why we don't do the same for it and any potential bugs or things that users have to look out for.
 
> However, I think in that case the ShellApp would never get assigned to the
> NotificationDaemon.Source, which is a bug.
>
> > -   * get the first plugged notification is to be able to get the WM_CLASS
> > -   * from the child window.  But we don't want to emit this signal twice
> > -   * if for some reason the socket gets replugged.
> > 
> > Did this have a point? If it had a point, was it replaced in your patch in some
> > way?
> 
> we disconnect from the signal now after its emitted, rather than just ignoring
> future emissions.

Ah.

(In reply to comment #18)
> Review of attachment 170270 [details] [review]:
> 
> Don't like having a debug mode change the actor hierarchy. Why can't you do
> this out of paint() and just paint the box there? Cogl isn't a great drawing
> API but it's easy enough to use for simple stuff like rectangle.

My first attempt at this didn't work; note that debug_box is a child of the stage, not a child/parent/peer of the ShellGtkEmbed, and the cogl-based version would have to behave the same way, drawing its rectangle in stage coordinates (because the entire point of drawing the rectangle is to verify that we are putting the ShellEmbeddedWindow at the same screen position as its ShellGtkEmbed, so we can't draw it in terms of gtkembed coordinates). But the way painting works in clutter makes this difficult, since we'd need to undo the transforms and clips applied by all of the gtkembed's ancestors before we can properly paint on the stage. (I thought just pushing an identity matrix would dtrt, but it doesn't seem to.)

Anyway, the debug mode was useful while I was fixing the positioning bug, but it's not really that useful now that it's fixed, so I'm going to just remove this patch.

(In reply to comment #21)
> Maybe it makes sense to have some function to insert a source that does the
> _sources[key] and the connection to remove the key, since you have that in
> three places?

It's two now (see below), and one is "add source to _sources and remove it when the source is destroyed" and the other is "add sender-to-pid mapping to _senderToPid and remove it when the source is destroyed", so they don't really combine easily.

> +        if (!source && hints['window-xid'])
> +            source = this._sources[hints['window-xid']];
> 
> Hmmm, this is to save a D-Bus round-trip?

Yeah... and that's not really worth it. It hadn't really occurred to me when I first started out that I could just always use the pid as the key and not worry about indexing window-xids. So I'm just getting rid of that.

> +  if ((xev->xany.type == EnterNotify || xev->xany.type == LeaveNotify)
> 
> This magic workaround needs a comment. (I can explain it if you need it)

Sure, why don't you write a comment for it, to make sure it's accurate.

Created attachment 170715 [details] [review]
[ShellGtkEmbed] base this on ClutterX11TexturePixmap, not ClutterGLX

As of 1.4, ClutterGLXTexturePixmap is identical to
ClutterX11TexturePixmap (with the differences having been moved into
the cogl layer). So make ShellGtkEmbed use the (introspected) X11
version, which then allows us to make the instance and class structs
public as well.

Created attachment 170716 [details] [review]
[ShellGtkEmbed] do a better job of tracking the actor's position

now computes the position manually rather than using get_transformed_position

Created attachment 170717 [details] [review]
[ShellTrayIcon] add ShellTrayIcon, make ShellTrayManager use it

removed a few properties we don't actually need. In particular, the
ShellApp stuff is gone now, because the JS can just figure that out from
the pid.

Created attachment 170718 [details] [review]
[MessageTray] show newly-added sources even if they don't post notifications

eg, when a trayicon is added, show the summary briefly

Created attachment 170719 [details] [review]
[StatusIconDispatcher] Move non-system-tray icons to message tray

New StatusIconDispatcher dispatches icons to the system or message tray.

Based on a patch from Matt Novenstern

Review of attachment 170715 [details] [review]:

Looks good

Review of attachment 170716 [details] [review]:

> now computes the position manually rather than using get_transformed_position

Is this the patch you wanted to attach here? this seems to use get_transformed_position()

Review of attachment 170717 [details] [review]:

Looks good to me

::: src/shell-embedded-window.c
@@ +234,3 @@
+  if (actor &&
+      CLUTTER_ACTOR_IS_REALIZED (actor) &&
+      gtk_widget_get_visible (GTK_WIDGET (window)))

probably should be called out in the commit message (and also the addition of PROP_STAGE) - these are conceptually separate changes, and while I don't think they need to be separate commits, they aren't really explained by the overall commit message.

::: src/shell-tray-icon.c
@@ +76,3 @@
+                               &type, &format, &nitems,
+                               &bytes_after, (guchar **)&val);
+  if (!gdk_error_trap_pop () &&

You don't actually need to check the error code from gdk_error_trap_pop() - if XGetWindowProperty() failed result would have been != Success - doesn't hurt though

Review of attachment 170718 [details] [review]:

Looks good

Created attachment 170782 [details] [review]
[ShellGtkEmbed] do a better job of tracking the actor's position

hm, i guess i lost the change in a rebase. go go gadget reflog...

Review of attachment 170719 [details] [review]:

Reads well, though it's certainly possible that there is some subtle detail of tray icon operation that I'm missing. If there is, and it matters, by definition we'll find it later :-)

::: src/gnome-shell-plugin.c
@@ +508,3 @@
 #endif
 
+  if ((xev->xany.type == EnterNotify || xev->xany.type == LeaveNotify)

/* When the pointer leaves the stage to enter a child of the stage
 * (like a notification icon), we don't want to produce Clutter leave
 * events. But Clutter treats all leave events identically, so we
 * need hide the detail = NotifyInferior events from it.
 *
 * Since Clutter doesn't see any event at all, this does mean that
 * it won't produce an enter event on a Clutter actor that surrounds
 * the child (unless it gets a MotionNotify before the Enter event).
 * Other weirdness is likely also possible.
 */

Review of attachment 170782 [details] [review]:

Looks good

Attachment 170715 [details] pushed as 745e8f6 - [ShellGtkEmbed] base this on ClutterX11TexturePixmap, not ClutterGLX
Attachment 170717 [details] pushed as ae93606 - [ShellTrayIcon] add ShellTrayIcon, make ShellTrayManager use it
Attachment 170718 [details] pushed as 82e6f9c - [MessageTray] show newly-added sources even if they don't post notifications
Attachment 170719 [details] pushed as cbf2cba - [StatusIconDispatcher] Move non-system-tray icons to message tray
Attachment 170782 [details] pushed as 63f2066 - [ShellGtkEmbed] do a better job of tracking the actor's position