Created attachment 171182 [details] [review]
Fix compilation against latest GTK+-3 changes

Adjust to GDK/GTK+ API changes:
 - removal of GdkColormap
 - expose-event -> draw transition

Review of attachment 171182 [details] [review]:

::: src/shell-tray-manager.c
@@ +247,3 @@
+      gdk_window_set_background_pattern (gtk_widget_get_window (widget),
+                                         bg_pattern);
+      cairo_pattern_destroy (bg_pattern);

This code does not set a surface as the background but sets a color. That looks wrong with the comment above. You'd need to use a surface pattern to get GDK to use XSetWindowBackPixmap.

Though I'm not sure if it's actually  necessary to use a pixmap to get ParentRelative to work right. I suppose Owen knows.

::: src/tray/na-tray-child.c
@@ +212,3 @@
+                                          floor (x1), floor (y1),
+                                          ceil (x2) - floor (x1),
+                                          ceil (y2) - floor (y1));

You should mention somewhere that this is copy/paste from GtkTrayIcon.

Also, both code bases should probably use gdk_cairo_get_clip_rectangle() instead of cairo_clip_extents() here.

@@ -278,3 @@
 
-  gtk_widget_set_colormap (GTK_WIDGET (child), colormap);
-

This code omits setting a custom visual. Using this should be done in the widget's realize function when creating the socket's GdkWindow.

Created attachment 171213 [details] [review]
Fix compilation against latest GTK+-3 changes

(In reply to comment #2)
> -  gtk_widget_set_colormap (GTK_WIDGET (child), colormap);
> -
> 
> This code omits setting a custom visual. Using this should be done in the
> widget's realize function when creating the socket's GdkWindow.

It would be nice if overwriting the socket's realize function could be avoided. I'm not sure how valid the approach in this patch is though - I use gtk_widget_set_visual() on the socket's window (ShellGtkEmbed) with a visual chosen according to na_tray_child_has_alpha().

Review of attachment 171182 [details] [review]:

Don't see anything additional other than the areas that Benjamin commented on, but I don't think the fix gtk_widget_set_colormap() is as straightforward as Benjamin implies - I think it's going to need functionality restored in GTK+ that was removed.

::: src/shell-tray-manager.c
@@ +247,3 @@
+      gdk_window_set_background_pattern (gtk_widget_get_window (widget),
+                                         bg_pattern);
+      cairo_pattern_destroy (bg_pattern);

Hmm, looking at the Xlib docs it appears that probably using the 1x1 pixmap was unecessary:

 If you specify a back-
 ground-pixel, it overrides either the default background-pixmap or any value you may have set in
 the background-pixmap. A pixmap of an undefined size that is filled with the background-pixel is
 used for the background.

While I"m sure we still have some apps using non-RGBA tray icons, I'm not sure which ones. (Actually, hmm, it should be very obvious now if the app is doing that, since we are showing icons against transparent in their new location - we'll likely see black squares behind old fashioned tray icons. If you can locate such an app quickly, try changing the bg-color to magenta and make sure it follows, otherwise, we'll just assume that the color + ParentRelative thing is working right)

::: src/tray/na-tray-child.c
@@ +203,2 @@
       /* Clear to parent-relative pixmap */
+      cairo_surface_flush (target);

The comment here is now weirdly positioned. Maybe just needs a blank line is needed. Certainly some explanation of what's going on with the flushing, etc, would almost certainly be a good idea. I basically can figure it out, but I think the average developer hitting this code would be entirely mystified.

@@ -278,3 @@
 
-  gtk_widget_set_colormap (GTK_WIDGET (child), colormap);
-

The removal of the ability to set the GdkVisual of a widget is a huge problem here - there's no way we can replace GtkSocket's realize function - it does stuff that is private to GTK+ (and should be private.) We need new GTK+ functionality of some sort to make this code work again. (gtk_socket_set_visual() is possible, but I'd rather see gtk_widget_set_visual(). I don't know if there are a lot of other cases where something like this is necessary, but I can't imagine it's the only place in the GNOME stack.)

The only workaround I can think of without GTK+ changes here would be to change this widget from is-a GtkSocket to has-a GtkSocket and introduce an extra X window into the hierarachy.

Created attachment 171288 [details] [review]
Fix compilation against latest GTK+-3 changes

Use gtk_widget_set_visual() to restore the old logic of setting the colormap/visual.

Review of attachment 171288 [details] [review]:

Couple of comments

::: src/shell-tray-manager.c
@@ +241,3 @@
+                                       GDK_WINDOW_XID (window),
+                                       GDK_VISUAL_XVISUAL (visual),
+                                       1, 1);

My conclusion in my last comments was that you didn't need to do this and that a color should work (unless experience proves that wrong)

::: src/tray/na-tray-child.c
@@ +212,3 @@
+                  GDK_WINDOW_XID (window),
+                  floor (clip_rect.x), floor (clip_rect.y),
+                  floor (clip_rect.width), floor (clip_rect.height),

Don't need the floor with GdkRectangle which is ints. (And using floor for all would be wrong if they weren't ints, would be more complicated - like ceil(x + width) - floor(x))

Created attachment 171297 [details] [review]
Fix compilation against latest GTK+-3 changes

(In reply to comment #6)
> My conclusion in my last comments was that you didn't need to do this and that
> a color should work (unless experience proves that wrong)

OK, reverted to the 1st patch's version - I don't know any application to test the correct behavior with though.


> Don't need the floor with GdkRectangle which is ints. (And using floor for all
> would be wrong if they weren't ints, would be more complicated - like ceil(x +
> width) - floor(x))

Right, this was copied from gtk+ (which uses cairo_clip_extents()) and then updated to use gdk_cairo_get_clip_rectangle() ...

Review of attachment 171297 [details] [review]:

Looks good. We're pretty broken anyways right now for non-ARGB tray icons (pop up on rectangles), and it doesn't matter a whole lot if they pop up on gray rectangles rather than black rectangles, so I'm willing to risk trusting the Xlib docs.

Attachment 171297 [details] pushed as fe5a289 - Fix compilation against latest GTK+-3 changes