Created attachment 160714 [details] [review]
Use accessor functions instead of direct access

With the transition to GTK+ 3.0, direct access to struct members
will no longer be possible.
To replace all calls to deprecated code, GTK+ 2.20 is required - due
to some basic compatibility code, it is still possible to build the
shell with GTK+ 2.18 when not using -DGSEAL_ENABLE.

Most changes should be pretty trivial, I'd be interested on input on the following though:

1. Compatibility with GTK+ 2.18:
   GTK+ 2.20 is still pretty new, so a straight dependency is probably not
   a good idea - I think the solution in this patch is pretty straightforward
   (and easy to remove when bumping the GTK+ requirement), nevertheless some
   comments would be nice.
2. Change in shell_embedded_window_show():
   I'm pretty sure I removed an optimization there, which is bad. I doubt it
   can be avoided, but I'd be glad to be proven wrong on that.

(Completely unrelated: it would be awfully cool if git-bz edit supported adding
 blocks/depends)

Review of attachment 160714 [details] [review]:

I think the way you handled 2.18 here is fine. I think we are going to have to bump to a GTK+-2.90 dependency quite soon, but until that happens I don't want to introduce a 2.20 dependency just for accessors.

::: src/gtk-compat.h
@@ +1,2 @@
+#ifndef __GTK_COMPAT_H__
+#define __GTK_COMPAT_H__

A comment here about the purpose of this file would be good.

@@ +6,3 @@
+#if !GTK_CHECK_VERSION(2, 20, 0)
+
+#define gtk_widget_get_realized(w) GTK_WIDGET_REALIZED((w))

Double parens here are unnecessary

::: src/shell-embedded-window.c
@@ +64,3 @@
     }
+
+  GTK_WIDGET_CLASS (shell_embedded_window_parent_class)->show (widget);

Not happy with this. This will mean round trips to the X server waiting for ConfigureNotify events to arrive (GtkWindow properly doesn't trust that gdk_window_resize() on a toplevel will be honored by the window manager), and a whole lot of complexity.

Since our current hack was taken away from us, we can add another. If we look at 
gtk_widget_real_show(), it is:

static void
gtk_widget_real_show (GtkWidget *widget)
{
  if (!gtk_widget_get_visible (widget))
    {
      GTK_WIDGET_SET_FLAGS (widget, GTK_VISIBLE);

      if (widget->parent &&
          gtk_widget_get_mapped (widget->parent) &&
          GTK_WIDGET_CHILD_VISIBLE (widget) &&
          !gtk_widget_get_mapped (widget))
        gtk_widget_map (widget);
    }
}

So, we can replace GTK_WIDGET_SET_FLAGS(widget, GTK_VISIBLE)

with:

 GtkWidgetClass *widget_class;

 /* Skip GtkWindow, but run the default GtkWidget handling which
  * marks the widget visible */
 widget_class = g_type_class_peek (GTK_TYPE_WIDGET);
 widget_class->show (widget);

Note that widget->parent is NULL for us (as I recall), so we need to do the widget_map() as we did in the old code - this just replaces the SET_FLAGS().

Also, if you want to pick up jjardon's Mutter patch (bug 595496), review the latest version, and do the same thing there as to 2.18 compatibility, that would be useful.

(What might make sense for committing it would be to commit it with the 2.20 dep, then do an immediate followup patch that adds back 2.18 compatibility.)

Created attachment 160733 [details] [review]
Use accessor functions instead of direct access

With the transition to GTK+ 3.0, direct access to struct members
will no longer be possible.
This bumps the required minimum version of GTK+ to 2.20.

Adress Owen's comment about shell_embedded_window_show() and fix some warnings in na-tray-child.c

Created attachment 160734 [details] [review]
Add compatibility with GTK+ 2.18

To replace all calls to deprecated code, GTK+ 2.20 is required - add
some basic compatibility code, so that is still possible to build the
shell with GTK+ 2.18 when not using -DGSEAL_ENABLE.

Split out from the previous patch version with Owen's comments adressed (hopefully)

Review of attachment 160733 [details] [review]:

looks good except for one tiny thing noted below.

My comments about separate commits were actually about the Mutter patch, not this one, up to you whether you commit this one split or squashed.

::: src/tray/na-tray-child.c
@@ +388,2 @@
   child->composited = composited;
+  if ((window = gtk_widget_get_window (GTK_WIDGET (child))))

We avoid this as a style thing - just put the assignment on a separate line.

Review of attachment 160734 [details] [review]:

Looks good (though can be squashed, see other comment.)

(In reply to comment #8)
> Looks good (though can be squashed, see other comment.)

Mmmh, when we bump the requirement to post 2.18, the split will allow us to remove the compatibility stuff with a simple git revert - or do you expect that code to stick around "forever"?

(In reply to comment #9)
> (In reply to comment #8)
> > Looks good (though can be squashed, see other comment.)
> 
> Mmmh, when we bump the requirement to post 2.18, the split will allow us to
> remove the compatibility stuff with a simple git revert - or do you expect that
> code to stick around "forever"?

Yes, that's a reasonable reason to keep the split.

Right now I'm thinking that we'll go for a hard 2.9x requirement within a few months, so hopefully there won't be intervening commits that interfere.

Attachment 160733 [details] pushed as 8c5bb86 - Use accessor functions instead of direct access
Attachment 160734 [details] pushed as 6318c8e - Add compatibility with GTK+ 2.18