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 580047 - Add size negotiation to the tray icons
Add size negotiation to the tray icons
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal minor
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-04-23 22:08 UTC by Milan Bouchet-Valat
Modified: 2009-05-12 18:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add size negotiation to the tray icons (37.02 KB, patch)
2009-05-03 21:13 UTC, Owen Taylor
none Details | Review

Description Milan Bouchet-Valat 2009-04-23 22:08:27 UTC
All icons in the notification area lack a few pixels at the top and at the bottom. Looks like the rounded square is too small for them. I guess everyone experiences this bug.
Comment 1 Owen Taylor 2009-05-03 21:12:45 UTC
I don't see exactly this, but I do see a two problems somewhat related:

 - The icons seem to be squished down by a few pixels, so appear to be
   missing some lines

 - Icons are 24x24 always, even when they don't want to be. Some of
   the gnome icons (gnome-power-manager, the packagekit icons), "hide"
   themselves by making themselves 1x1 and transparent, instead of removing
   from the tray area. (No idea why)

I've rewritten things to do real size negotiation for the tray icons and that fixes the above two problems for me. hopefully it will fix your problems as well. I'll attach a patch.
Comment 2 Owen Taylor 2009-05-03 21:13:17 UTC
Created attachment 133878 [details] [review]
Add size negotiation to the tray icons

* Rename ShellGtkWindowActor to ShellGtkEmbed, and make it require
  a ShellEmbeddedWindow rather than a general GtkWindow.

* Add ShellEmbeddedWindow subclass of GtkWindow that bypasses most
  of the GtkWindow complexity for size negotiation, and calls out
  to a clutter actor instead; also automatically handle reparenting
  the GtkWindow into the stage window.

* Use the reworked ShellGtkEmbed in ShellTrayManager, this simplifies
  the code a bit, and more importantly results in the tray icons
  having the correct size negotiation, rather than having a fixed
  24x24 size.
Comment 3 Dan Winship 2009-05-04 17:38:14 UTC
+/* This tpe is a a subclass of GtkWindow that ties the window to a

typo ("tpe")


+  GdkRectangle position;

Using a rectangle for a position?


+  /* The reason for "doing this behind GTK+'s back" rather than using
+   * gdk_window_reparent() is that there might not be a GDK window for the
+   * for the stage_xwindow, so this avoids gdk_window_foreign_new().
+   * GDK will be left thinking that the parent of window->window is
+   * the root window - it's not immediately clear to me whether that is
+   * more or less likely to cause problems than modifying the heirarchy.
+   */
+  XReparentWindow (GDK_DISPLAY_XDISPLAY (gtk_widget_get_display (widget)),

It's not like this happens in a tight loop or something. A round trip here (which I'm assuming is why you want to avoid gdk_window_foreign_new()) won't be noticed.

Also, "hierarchy".


+static void
+shell_embedded_window_destroy (GtkObject *object)
+{
+  GTK_OBJECT_CLASS (shell_embedded_window_parent_class)->destroy (object);
+}

Leftover? Or, if this is doing anything, it's subtle enough that it needs a comment.
Comment 4 Owen Taylor 2009-05-04 18:27:33 UTC
(In reply to comment #3)
> +/* This tpe is a a subclass of GtkWindow that ties the window to a
> 
> typo ("tpe")

Fixed. And the other typo on the same line.
 
> +  GdkRectangle position;
> 
> Using a rectangle for a position?

It's somewhere between a left-over and future-proofing. My feeling was that storing the complete dimensions of the computed position was better than storing part of it and, say, accessing the widget->allocation.width/height for the rest. But it turned out that only the x/y were actually needed.

I've left it as is.
 
> +  /* The reason for "doing this behind GTK+'s back" rather than using
> +   * gdk_window_reparent() is that there might not be a GDK window for the
> +   * for the stage_xwindow, so this avoids gdk_window_foreign_new().
> +   * GDK will be left thinking that the parent of window->window is
> +   * the root window - it's not immediately clear to me whether that is
> +   * more or less likely to cause problems than modifying the heirarchy.
> +   */
> +  XReparentWindow (GDK_DISPLAY_XDISPLAY (gtk_widget_get_display (widget)),
> 
> It's not like this happens in a tight loop or something. A round trip here
> (which I'm assuming is why you want to avoid gdk_window_foreign_new()) won't be
> noticed.

I think my concern was less the round trip than the side effect of creating a foreign window when I just wanted to reparent one X window into another. And the lookup/foreign_new/deal-with-memory-management hoops are always annoying. So, in essence the XReparentWindow call is just the cleaner and simpler way to do it. *But* I wanted to leave a comment in case fooling GDK about the 
hierarchy caused problems later. I'm basically uncertain what is better.

I left the code as is, and rewrote the comment to better represent the situation:

  /* Using XReparentWindow() is simpler than using gdk_window_reparent(),                              
   * since it avoids maybe having to create a new foreign GDK window for                               
   * the stage. However, GDK will be left thinking that the parent of                                  
   * window->window is the root window - it's not immediately clear                                    
   * to me whether that is more or less likely to cause problems than                                  
   * modifying the GDK hierarchy.                                                                      
   */

> Also, "hierarchy".

Fixed.

> +static void
> +shell_embedded_window_destroy (GtkObject *object)
> +{
> +  GTK_OBJECT_CLASS (shell_embedded_window_parent_class)->destroy (object);
> +}
> 
> Leftover? Or, if this is doing anything, it's subtle enough that it needs a
> comment.

Leftover, fixed.

Pushed with the changes noted above.
Comment 5 Owen Taylor 2009-05-12 18:39:31 UTC
Forgot to close this earlier.