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 598313 - use a stable ordering for core OS tray icons
use a stable ordering for core OS tray icons
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-10-13 18:38 UTC by Colin Walters
Modified: 2009-10-19 19:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[panel] Use a fixed ordering for well-known icons (8.41 KB, patch)
2009-10-15 21:30 UTC, Colin Walters
reviewed Details | Review
Use a fixed ordering for well-known icons (7.56 KB, patch)
2009-10-16 16:19 UTC, Colin Walters
none Details | Review
Use a fixed ordering for well-known icons (7.43 KB, patch)
2009-10-16 17:03 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2009-10-13 18:38:33 UTC
<mccann> walters:  transient ones, lang/keyboard, volume, bluetooth, network, battery  we think
<mccann> when reading from left to right
Comment 1 Colin Walters 2009-10-15 21:30:47 UTC
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
Comment 2 William Jon McCann 2009-10-16 00:53:25 UTC
Works great!  I tested with a few transient status icons like the evolution new mail notifier too.  A very nice improvement.
Comment 3 Dan Winship 2009-10-16 12:51:38 UTC
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.)
Comment 4 Colin Walters 2009-10-16 16:19:35 UTC
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
Comment 5 Owen Taylor 2009-10-16 16:55:03 UTC
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?
Comment 6 Colin Walters 2009-10-16 17:03:59 UTC
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
Comment 7 Dan Winship 2009-10-19 17:35:49 UTC
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
Comment 8 Colin Walters 2009-10-19 19:02:08 UTC
(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...
Comment 9 Colin Walters 2009-10-19 19:03:12 UTC
Attachment 145625 [details] pushed as 7f5c600 - Use a fixed ordering for well-known icons