Created attachment 145555 [details] [review]
[panel] Use a fixed ordering for well-known icons

Define the ordering for well-known icons; see
http://live.gnome.org/Features/StandardIconOrdering

Works great!  I tested with a few transient status icons like the evolution new mail notifier too.  A very nice improvement.

Review of attachment 145555 [details] [review]:

Basically good. I think the STANDARD_TRAY_ICON_ORDER stuff makes sense.

::: js/ui/panel.js
@@ +454,3 @@
+            let position;
+            for (let i = 0; i < STANDARD_TRAY_ICON_ORDER.length; i++) {
+                if (role == STANDARD_TRAY_ICON_ORDER[i]) {

position = STANDARD_TRAY_ICON_ORDER.indexOf(role)

::: src/shell-tray-manager.c
@@ +5,3 @@
 #include <gtk/gtk.h>
 
+#include "display.h"

That's mutter's display.h? Should use <> rather than "" to make it clear that it's external

@@ +134,3 @@
+
+  screen = shell_global_get_screen (shell_global_get ());
+  display = meta_screen_get_display (screen);

(in shell_tray_manager_init)
this looks like leftovers from an earlier version of the patch; you assign the variables and then don't use them

@@ +166,3 @@
 		  G_STRUCT_OFFSET (ShellTrayManagerClass, tray_icon_added),
 		  NULL, NULL,
+		  gi_cclosure_marshal_generic,

Hm... I see we're already using that in some other places. Should probably just change everything over at some point. (I agree that if we eventually want to use it everywhere, it makes sense to do this change as part of this patch, rather than creating a new shell-marshal marshaller just so we can get rid of it later.)

@@ +297,3 @@
+  display = meta_screen_get_display (screen);
+
+  XGetClassHint (meta_display_get_xdisplay (display), GDK_WINDOW_XWINDOW (window), &class_hint);

I think we need a push_error_trap/pop_error_trap around this to protect against race conditions like if the notification icon crashes right after adding itself. Owen?

And if so, then also check the return value of XGetClassHint, and not free class_hint's fields in that case.

Also, XFree can't take NULL as an argument, so the checks below seem inconsistent. (You indirectly test if class_hint.res_class was NULL, but then XFree it anyway.)

Created attachment 145621 [details] [review]
Use a fixed ordering for well-known icons

Define the ordering for well-known icons; see
the page http://live.gnome.org/Features/StandardIconOrdering

Review of attachment 145621 [details] [review]:

Looking just at the XGetClassHint part (since there was a question about that)

::: src/shell-tray-manager.c
@@ +293,3 @@
+  gdk_error_trap_push ();
+
+  success = XGetClassHint (meta_display_get_xdisplay (display), GDK_WINDOW_XWINDOW (window), &class_hint);

Yes, the error trap was needed here

@@ +297,3 @@
+  gdk_error_trap_pop ();
+
+  result = g_strdup (class_hint.res_class);

You can't use class_hint.res_class if success is 0 - it has undefined contents; it's not reliably NULL. (If you nulled it it out before you could, but I think that would just be more confusing than having success and failure code paths)

@@ +302,3 @@
+  else
+    result_lower = NULL;
+  g_free (result);

Why strdup something then immediately free it?

Created attachment 145625 [details] [review]
Use a fixed ordering for well-known icons

Define the ordering for well-known icons; see
the page http://live.gnome.org/Features/StandardIconOrdering

Review of attachment 145625 [details] [review]:

1 new comment and 3 that I missed before... probably good to just commit after addressing them though

::: src/shell-tray-manager.c
@@ +161,3 @@
+		  gi_cclosure_marshal_generic,
+		  G_TYPE_NONE, 2,
+          CLUTTER_TYPE_ACTOR,

looks like the two signal existing definitions use tabs for indentation, but the rest of the file consistently uses just spaces. Can you fix that while you're there?

@@ +314,3 @@
+  child = g_hash_table_lookup (manager->priv->icons, socket);
+  if (child->emitted_plugged)
+    return;

you're not setting emitted_plugged anywhere

@@ +318,3 @@
+  wm_class = get_lowercase_wm_class_from_socket (manager, socket);
+  g_signal_emit (manager, shell_tray_manager_signals[TRAY_ICON_ADDED], 0,
+                 child->actor, wm_class);

we probably want to bail out rather than emitting TRAY_ICON_ADDED if this returns NULL, since that probably (definitely?) means the icon isn't actually there any more

@@ +361,3 @@
   g_hash_table_insert (manager->priv->icons, socket, child);
 
+  g_signal_connect (socket, "plug-added", G_CALLBACK(on_plug_added), manager);

space after G_CALLBACK

(In reply to comment #3)
> 
> Hm... I see we're already using that in some other places. Should probably just
> change everything over at some point. (I agree that if we eventually want to
> use it everywhere, it makes sense to do this change as part of this patch,
> rather than creating a new shell-marshal marshaller just so we can get rid of
> it later.)

I'd like to really get this into glib this cycle.  We'll see...

Attachment 145625 [details] pushed as 7f5c600 - Use a fixed ordering for well-known icons