Created attachment 167552 [details] [review]
[WIP] Replace Gdk drawing API with cairo

Just applied the patch, and it seems to work just fine.  Everything shows up in the right spot, frames are there, etc.  Are you planning to tackle theme.c?  I'm looking at the code right now, but I don't want to mess with it if it's already being taken care of.

Created attachment 167750 [details] [review]
Replace GDK drawing API with cairo

Updated patch to replace all removed API calls.

Still, there are a bunch of problems/questions:

 - the replacement for gdk_draw_pixbuf() in render_pixbuf()
   in ui/theme.c is wrong - with Clearlooks, I get messed up
   title bars for any window which is not gnome-terminal
   (if that doesn't make sense - I agree)

 - the replacement for gdk_draw_arc() is untested in the wild
   (none of the common themes seems to use it).
   If desired, I can attach a small test program though

 - I'm unaware of any replacements for gdk_screen_get_rgb(a)_colormap,
   so I just removed the calls

Apart from the complete render_pixbuf() mess, there are some minor glitches notable on rounded corners (which the theme implements using lines - ugh).

Created attachment 167751 [details] [review]
Replace GDK drawing API with cairo

Sorry for the noise, I forgot to squash a small fix. All comments above still apply of course.

Review of attachment 167751 [details] [review]:

OK, I didn't find the problem drawing the clearlooks titlebars reading through the code (or at least not obviously), what I did find as:

 - Style stuff
 - Some off-by-ones
 - Major breakage for arcs, which also look like they were majorly broken before

::: src/ui/draw-workspace.c
@@ +199,3 @@
   if (workspace_background)
     {
+      cr = gdk_cairo_create (drawable);

Should share the cr with the else

@@ +208,1 @@
       cr = gdk_cairo_create (gtk_widget_get_window (widget));

Hmm, this seems to have been wrong - should have been drawable like the above

::: src/ui/frames.c
@@ +2040,3 @@
 {
+  int width, height;
+  cairo_t *cr = gdk_cairo_create (GDK_DRAWABLE (pixmap));

Don't need this cast, or any of the GDK_DRAWABLE casts, GdkWindow and GdkPixmap are typedefs for GdkDrawable, not separate structures

::: src/ui/tabpopup.c
@@ +750,2 @@
 #endif
+      cairo_set_line_width (cr, 2.0);

This seems to have been reordered with respect to the original code in terms of the code that is there, and the code that was #if 0- of course, I'm not sure what the #if 0'ed code was about.

@@ +753,3 @@
+
+      cairo_rectangle (cr, x, y, w, h);
+      cairo_stroke (cr);

It looks like this is off-by-one, the total width of the previous drawing was w + 3, h + 3, and with your change, it's w + 2, h + 2. [The previous drawing may have been 1-pixel too wide/hugh, but I think it's better to assume that it was right and stay pixel identical.] So, w, h should be 1 more here.

::: src/ui/testgradient.c
@@ +122,3 @@
+  gdk_cairo_set_source_pixbuf (cr, pixbuf, 0, 0);
+  cairo_rectangle (cr, 0, 0, width, height);
+  cairo_fill (cr);

Can you use the cr that was passed in?

@@ +252,3 @@
 
+  window = gtk_widget_get_window (widget);
+  cr = gdk_cairo_create (GDK_DRAWABLE (window));

More GDK_DRAWABLE casts (I haven't tried to comment them comprehensively)

::: src/ui/theme.c
@@ +2924,3 @@
 
+static cairo_t *
+get_cr_for_primitive (GtkWidget          *widget,

I'd rather see this as setup_cr_for_primitive() like you did elsewhere. To me, the right compromise between doing it right (using a single cr) and getting things compiling and working is to create a cr in meta_draw_op_draw_with_env, set a clip on it, then pass it in to the subordinate drawing functions.

(Long term, you want to pass the cr *to* draw_op_with_env, but probably good to limit the scope of refactoring at this point.)

@@ +2942,3 @@
   if (clip)
+    {
+      gdk_cairo_rectangle (cr, (GdkRectangle*) clip);

Cast isn't necessary - gdk_cairo_rectangle() takes a const rectangle

@@ +3509,3 @@
             op->data.line.width==0)
+          {
+            cairo_set_line_cap (cr, CAIRO_LINE_CAP_ROUND);

gdk_draw_point() doesn't draw a nice dot, it draws a 1x1 pixel square pixel. You should do the same here using cairo_rectangle().

@@ +3526,3 @@
 
+            cairo_move_to (cr, (double)x1 + .5, (double)y1 + .5);
+            cairo_line_to (cr, (double)x2 + .5, (double)y2 + .5);

OK, this is a bit tricky. What I think we should strive for is:

 - Horizontal and vertical lines are pixel-per-pixel identical to what you'd get with X
 - Other lines are unspecified and hopefully pretty.

You can work out the rasterization rules for horizontal and vertical rules yourself using section 7.1 of the Xlib manual and some graph paper, but to skip to the result,
here's the mapping from Xlib lines (using the default BUTT cap) to Cairo lines (using the default BUTT cap)

 Horizontal line:
  Odd width:  (x1, y + 0.5) to (x2, y + 0.5)
  Even width: (x1, y) to (x2, y)

 Vertical line:
  Odd width:  (x + 0.5, y1) to (x + 0.5, y2)
  Even width: (x, y1) to (x, y2)

(The cairo line width equals the X line width)

Note that the coordinates perpendicular to the line get offset, but the coordinates parallel don't. I can't think of anything reasonable to do with diagonal lines, so I'd just draw them from x1,y1 to x2,y2.

Since we are already deciphering horizontal and vertical lines and worrying about the pixels they cover, I think the simplest way and most efficient way to turn that into code is:

 /* We reproduce the exact X rasterization rules for horizontal
  * and vertical lines with BUTT caps by drawing them as rectangles;
  * for other lines, we just use the normal Cairo rasterization
  * rules.
  */
 if (y1 == y2)
   {
      cairo_rectangle (cr, x1, y1 - w / 2, x2 - x1, w);
      cairo_fill (cr);
   }
 else if (x1 == x2)
   {
     cairo_rectangle (cr, x1 - w / 2, y1, w, y2 - y1);
     cairo_fill (cr);
   }
 else
   {
     cairo_move_to (cr, x1, y1);
     cairo_line_to (cr, x2, y2);
     cairo_stroke (cr);
   }

(The 0.5 offsets above are covered by the integer w being divided by 2). Note w here needs to be 1 for theme format specified width 0. Might be best to just convert that when parsing the theme?

@@ +3547,3 @@
         rheight = parse_size_unchecked (op->data.rectangle.height, env);
 
+        cairo_rectangle (cr, (double)rx + .5, (double)ry + .5,

The double casts strike me as extraneous clutter - there's an implicit cast from adding integer to double.

@@ +3574,3 @@
 
+        start_angle = op->data.arc.start_angle * (M_PI / 180.)
+                      - (.5 * M_PI); /* start at 12 instead of 3 oclock */

This comment and code looks mostly right to me for the new version, but the old version as far as I can tell was completely busted. If we have a arc.start_angle = 0, then we get start angle = -90 degrees, which for X, is I believe, the positive y axis - 6 o'clock. And the direction looks backwards too. Quotes from the Xlib manual: "Specifies the start of the arc relative to the three-o’clock position from the center, in units of degrees * 64. "Positive angles indicate counterclockwise motion".

However, 0.25 * M_PI, not 0.5 * M_PI.

The theme format docs say:

 The values of these attributes are given in degrees clockwise, with 0 being straight up.

Let's implement that and assume that the code is either broken and never tested at all, or I'm misunderstanding the X spec.

@@ +3575,3 @@
+        start_angle = op->data.arc.start_angle * (M_PI / 180.)
+                      - (.5 * M_PI); /* start at 12 instead of 3 oclock */
+        extent_angle = -(op->data.arc.start_angle + op->data.arc.extent_angle)

I think this is wrong, it should be

  end_angle = start_angle + op->data.extent_angle * (M_PI / 180.)

(name change, sign, using start_angle to get the same 90 degree offset as the start angle)

@@ +3578,3 @@
+                       * (M_PI / 180.);
+        center_x = rx + (double)rwidth / 2. + .5;
+        center_y = ry + (double)rheight / 2. + .5;

Arcs. Woo. Excitement! Wasted a bit of graph paper here, thought about it a bit, and I think it basically boils down to:

 /* If the line-width is odd, then we should offset the horizontal lines
    at top and bottom and the vertical lines at left and right by half a pixel
    to pixel align them the same way as X. In other words, we offset the
    center by half a pixel in both directions. */

But only do this for odd line width. When drawing quarter arcs this isn't always going to pixel align the end pixels of the lines, but in the end, is about as good as makes any sense to do.

@@ +3580,3 @@
+        center_y = ry + (double)rheight / 2. + .5;
+
+        if (rwidth == rheight)

Unnecessary optimization - cairo-translate/cairo-scale aren't slow and it's better not to have two code paths for code that is barely tested to start with.

@@ +3595,3 @@
+            cairo_scale (cr, (double)rwidth / 2., (double)rheight / 2.);
+
+            if (op->data.arc.extent_angle >= 0)

With the interpretation of positive angles are clockwise for the theme - same as for cairo, positive extent_angle => cairo_arc, negative extent angle => cairo_arc_negative

@@ +3634,3 @@
 
+            cairo_rectangle (cr, (double)rx + .5, (double)ry + .5,
+                             rwidth, rheight);

0.5 offsets only for odd line width.

::: src/ui/ui.c
@@ +998,2 @@
   depth = gdk_drawable_get_depth (GDK_DRAWABLE (gpmap));
+  cmap = gdk_screen_get_system_colormap (screen);

You need to leave the call to get_rgba_colormap() here, it's just the rgb_colormap that should be replaced by the system colormap. [I think the removal of get_rgb_colormap() was probably a mistake, but rgba vs. system only matters for 8/24 displays, which are vanishingly rare these days, so get_system_colormap() is a good enough replacement. The rgba colormap is genuinely different on all systems.]

Created attachment 167799 [details] [review]
Replace GDK drawing API with cairo

Updated patch.

For the arc part I just updated the code according to the review, I didn't spent any time thinking about it or testing it - it appears to be pretty much unused, I can have a deeper look once the rest is fixed.

In regard to lines: using rectangles for horizontal/vertical lines, I suppose we'd have to implement dashed lines using masks? Or is that (afaik heavily unused) feature worth changing the implementation to cairo_stroke()?

I disabled antialias for now, to match metacity's output - we may consider turning it back on if we like the result better.


(In reply to comment #5)
>  if (y1 == y2)
>    {
>       cairo_rectangle (cr, x1, y1 - w / 2, x2 - x1, w);

To include x2 the rectangle width has to be x2 - x1 + 1?


>      cairo_move_to (cr, x1, y1);
>      cairo_line_to (cr, x2, y2);
>      cairo_stroke (cr);

I added .5 offsets here, as the result matches metacity better (at least for the themes I tried)


> @@ +3634,3 @@
> 
> +            cairo_rectangle (cr, (double)rx + .5, (double)ry + .5,
> +                             rwidth, rheight);
> 
> 0.5 offsets only for odd line width.

The line width is set to 1.0 in setup_cr_for_primitive() though - should I add the check anyway for clarity?

Review of attachment 167799 [details] [review]:

Looking close

::: src/ui/theme.c
@@ +3013,3 @@
+
+  gdk_cairo_set_source_pixbuf (cr, pixbuf, x, y);
+  gdk_cairo_rectangle (cr, &draw_rect);

Missed this in the last review, but this entire render_pixbuf() function can be reduced to:

  cr = gdk_cairo_create (drawable);
 
  gdk_cairo_set_source_pixbuf (cr, pixbuf, x, y);
  if (clip)
     {
      gdk_cairo_rectangle (cr, clip);
      cairo_fill (cr);
     }
   else
    cairo_paint (cr);

  cairo_destroy (cr);

Or really, now that you are creating and clipping the cr in meta_draw_op_draw_with_env, this function should be:

static void
render_pixbuf (cairo_t            *cr,
               GdkPixbuf          *pixbuf,
               int                 x,
               int                 y)
{
  gdk_cairo_set_source_pixbuf (cr, pixbuf, x, y);
  cairo_paint (cr);
}

Which probably doesn't even deserve to be a function, since the set_source()/paint() is the idiomatic way to draw images in cairo.

@@ +3531,3 @@
+                cairo_rectangle (cr,
+                                 x1, y1 - line_width / 2,
+                                 x2 - x1 + 1, line_width);

I just forgot about dashing when suggesting using cairo_rectangle, it's fine to write the line drawing as a stroke. Cairo will internally optimize to a rectangle when possible. (The coordinates of the lines were in my original comment:

 Horizontal line:
  Odd width: (x1, y + 0.5) to (x2, y + 0.5)
  Even width: (x1, y) to (x2, y)
 Vertical line:
 Odd width: (x + 0.5, y1) to (x + 0.5, y2)
 Even width: (x, y1) to (x, y2)

You probably want this even odd/even check if you turn off antialiasing. If you blindly add 0.5 to each coordinate than you'll get the same result, but it will be vastly slower, since it will go through the "render to an intermediate mask" code path rather than the "draw solid color rectangle" code path.

@@ +3545,3 @@
+                cairo_move_to (cr, x1 + .5, y1 + .5);
+                cairo_line_to (cr, x2 + .5, y2 + .5);
+                cairo_stroke (cr);

Yeah, if we turn off antialiasing, than the 0.5 offset is right - it should give almost identical rendering between Xlib and cairo. (Xlib samples at the upper left of pixels, cairo in the middle of pixels.)

For antialiased rendering I guess probably want that you want that 0.5 offset as well for maximum Xlib consistency - the argument being that the 255 samples that cairo does for an antialiased pixel have an average position at the center of the pixel, like the single sample in the antialiased case.

But you definitely don't want that 0.5 offset generically for horizontal vertical lines - you have to add the 0.5 only when you need an 0.5 or you'll get fuzzy and slow lines.
(Or with antialiasing off, just slow lines)

My concern with turning antialiasing off is that:

 A) It's ugly to have antialiasing off
 B) It's potentially really, really, slow to have antialiasing off. (Basically, cairo implements A1 as packed masks, but modern hardware doesn't know how to handle packed bitmasks. Some drivers *might* expand A1 to A8 on upload into video memory, but I don't think that's common across our driver base.)

Are there themes that you've found that use sloped lines? I'd say if there are common themes that use sloped lines and we look at them, and they look bad with antialiasing on, then we should turn it off, but otherwise, I'd rather have it on, because antialiasing on is the tested, optimized code path.

@@ +3564,3 @@
         rwidth = parse_size_unchecked (op->data.rectangle.width, env);
         rheight = parse_size_unchecked (op->data.rectangle.height, env);
+        offset = op->data.rectangle.filled ? 0. : .5;

Needs to be dependent on line-width.

@@ +3588,3 @@
         rwidth = parse_size_unchecked (op->data.arc.width, env);
         rheight = parse_size_unchecked (op->data.arc.height, env);
+        offset = ((int)cairo_get_line_width (cr) % 2) ? .5 : 0.;

Not crazy about casting an int to a double, storing it in cairo, pulling it back out, truncating to an int and then taking the mod. Since integers have precise bit representations as doubles, it should work, but it's in the class of operations that cause problems. It would be better to do the check on the original integer line width.

Created attachment 167851 [details] [review]
Replace GDK drawing API with cairo

> Missed this in the last review, but this entire render_pixbuf() function can be
> reduced to:
> [...]
> Which probably doesn't even deserve to be a function, since the
> set_source()/paint() is the idiomatic way to draw images in cairo.

Right - I removed the function, and did the same with setup_cr_for_primitive(). I think that by calling cairo_set_line_width (cr, 1.0) when setting up the context and updating the line width for META_DRAW_LINE it becomes more obvious that all other primitives use a fixed line width. With that change the function is reduced to:

static void
setup_cr_for_primitive (GtkWidget     *widget,
                        MetaColorSpec *color_spec)
{
  GdkColor color;

  meta_color_spec_render (color_spec, widget, &color);
  gdk_cairo_set_source_color(cr, &color);
}

Which doesn't really deserve to be a function either :)


> Are there themes that you've found that use sloped lines? I'd say if there are
> common themes that use sloped lines and we look at them, and they look bad with
> antialiasing on, then we should turn it off, but otherwise, I'd rather have it
> on, because antialiasing on is the tested, optimized code path.

Some themes (including Clearlooks) use sloped lines to draw the 'X' on the close button. It looks a bit blurred with antialiasing on, but not particularly bad (IMHO), so antialiasing is back on.


> @@ +3564,3 @@
>          rwidth = parse_size_unchecked (op->data.rectangle.width, env);
>          rheight = parse_size_unchecked (op->data.rectangle.height, env);
> +        offset = op->data.rectangle.filled ? 0. : .5;
> 
> Needs to be dependent on line-width.

The line width is always 1 (or more exactly, the old code set it to 0).


> @@ +3588,3 @@
>          rwidth = parse_size_unchecked (op->data.arc.width, env);
>          rheight = parse_size_unchecked (op->data.arc.height, env);
> +        offset = ((int)cairo_get_line_width (cr) % 2) ? .5 : 0.;
> 
> Not crazy about casting an int to a double, storing it in cairo, pulling it
> back out, truncating to an int and then taking the mod. [...]
> It would be better to do the check on the original integer line width.

There is no original integer line width - I don't see an alternative to either use casting+mod or take advantage of the knowledge that the line width is fixed to 1. I updated the code to match the META_DRAW_RECTANGLE code, assuming that we only want offsets when using cairo_stroke() - is that correct?

Created attachment 167945 [details] [review]
Remove setup_bg_cr() due to GDK changes

Both gdk_window_get_background() and gdk_window_get_back_pixmap()
have been removed from GDK without replacement.

See http://git.gnome.org/browse/gtk+/commit/?id=0b29f4e7

Review of attachment 167851 [details] [review]:

Looks good except for some style stuff and 0.5 pixels for filled arcs.

::: src/ui/theme.c
@@ +3408,3 @@
                             MetaRectangle        rect,
                             MetaPositionExprEnv *env)
 {

Here's a overall comment you can use at the top of draw_op_with_env()

/* This code was originally rendering anti-aliased using X primitives, and
 * now has been switched to draw anti-aliased using cairo. In general, the
 * closest correspondence between X rendering and cairo rendering is given
 * by offsetting the geometry by 0.5 pixels in both directions before rendering
 * with cairo. This is because X samples at the upper left corner of the
 * pixel while cairo averages over the entire pixel. However, in the cases
 * where the X rendering was an exact rectangle with no "jaggies"
 * we need to be a bit careful about applying the offset. We want to produce
 * the exact same pixel-aligned rectangle, rather than a rectangle with
 * fuzz around the edges.
 */

(Visual explanation at http://picasaweb.google.com/lh/photo/ernzgK-VEEgTEYZdtkTP40jSsOPdthW_ne3SeHz_Qk4?feat=directlink - you could put the link in the source code and hopefully it will work for at least a few years. Don't think ASCII art would work.)

@@ +3456,3 @@
           {
+            double offset = (op->data.line.width == 0 ||
+                             op->data.line.width % 2) ? .5 : 0;

I think this should move inside an 'if (x1 == x2 || y1 == y2)', along with a comment:

 /* This one of the cases where we are matching the exact
  * pixel aligned rectangle produced by X.
  */

@@ +3471,3 @@
+             * and vertical lines with BUTT caps by offsetting odd lines;
+             * for sloped lines, we always use an offset to match Xlib.
+             */

Then this comment would go away.

@@ +3504,3 @@
         rwidth = parse_size_unchecked (op->data.rectangle.width, env);
         rheight = parse_size_unchecked (op->data.rectangle.height, env);
+        offset = op->data.rectangle.filled ? 0. : .5;

I think it would be clearer if there wasn't a variable and you just meged this with the if and had two different cairo_rectangle calls. And a comment:

 /* Filled and stroked rectangles are the other cases 
    we pixel-align to X rasterization */

@@ +3527,3 @@
         rwidth = parse_size_unchecked (op->data.arc.width, env);
         rheight = parse_size_unchecked (op->data.arc.height, env);
+        offset = op->data.arc.filled ? 0. : .5;

Actually you want an unconditional offset, I think. It will mean that filled rectangles will be slightly fuzzier at the sides and top/bottom, but an antialiased rectangle is inherently fuzzy, and there will be a better alignment with the X results. The filled rectangle is the case I drew in the whiteboard picture.

Created attachment 167995 [details] [review]
Replace GDK drawing API with cairo

Updated patch according to review.

Created attachment 167996 [details] [review]
Use gdk_window_get_background_pattern() to clear the background

gdk_window_get_back_pixmap() and gdk_window_get_background() have
been removed/deprecated. Use gdk_window_get_background_pattern()
as replacement.

Review of attachment 167995 [details] [review]:

Looks good

Review of attachment 167996 [details] [review]:

::: src/ui/frames.c
@@ -2006,3 @@
 
-static void
-setup_bg_cr (cairo_t *cr, GdkWindow *window, int x_offset, int y_offset)

Removing the offset isn't right

@@ +2054,3 @@
+    setup_bg_cr (cr, parent);
+  else if (bg_pattern)
+    cairo_set_source (cr, bg_pattern);

what you want to do to handle the offset is

 cairo_save (cr);
 cairo_translate (cr, - x_offset, - y _offset);
 cairo_set_source (cr, bg_pattern);
 cairo_restore (cr);

Review of attachment 167996 [details] [review]:

::: src/ui/frames.c
@@ +2005,3 @@
 }
 
+#if !GTK_VERSION(2,21,6)

Compile error.

I believe you want:

#if !GTK_CHECK_VERSION (2, 21, 6)

Review of attachment 167995 [details] [review]:

::: src/ui/theme.c
@@ +3487,3 @@
+                  {
+                    cairo_move_to (cr, x1, y1 + offset);
+                    cairo_line_to (cr, x2 + 1, y2 + offset);

I cannot really figure out the +1 offset to x2 (or y2 for vertical lines), but it fixes quite a few issues (in Clearlooks: the window frame and button borders). On the other hand, it overdraws when used in combination with rectangles (in Clearlooks: a horizontal line at the top of the maximize button). Owen, can you take a look at this?

Created attachment 168046 [details] [review]
Use gdk_window_get_background_pattern() to clear the background

(In reply to comment #14)
> Removing the offset isn't right
> 
> @@ +2054,3 @@
> +    setup_bg_cr (cr, parent);
> +  else if (bg_pattern)
> +    cairo_set_source (cr, bg_pattern);
> 
> what you want to do to handle the offset is
> 
>  cairo_save (cr);
>  cairo_translate (cr, - x_offset, - y _offset);
>  cairo_set_source (cr, bg_pattern);
>  cairo_restore (cr);

If I read the cairo source[0] correctly, the source pattern is part of the graphics context which is pushed/popped. So the updated patch uses a pattern of

  cairo_translate (cr, - x_offset, - y_offset);
  cairo_set_source (cr, bg_pattern);
  cairo_translate (cr, x_offset, y_offset);

instead, which should produce the correct result.


(In reply to comment #15)
> Compile error.

Yup, sorry for the typo!


[0] http://cgit.freedesktop.org/cairo/tree/src/cairo-gstate-private.h#n41

git apply Use-gdkwindowgetbackgroundpattern-to-clear-the-bac.patch

error: patch failed: src/ui/frames.c:2004
error: src/ui/frames.c: patch does not apply

sorry i can't patch mutter because error at comment #18. a made a wipe and refresh before patch

(In reply to comment #18)
> git apply Use-gdkwindowgetbackgroundpattern-to-clear-the-bac.patch
> 
> error: patch failed: src/ui/frames.c:2004
> error: src/ui/frames.c: patch does not apply

Did you apply the other patch first?

ok sorry i understand this was a review  with both patch

Review of attachment 168046 [details] [review]:

One small fix, otherwise, looks good to commit

::: src/ui/frames.c
@@ +2054,2 @@
     {
+      setup_bg_cr (cr, parent, x_offset, y_offset);

This should be, as it was before:

gdk_window_get_position (window, &window_x, &window_y);
setup_bg_cr (cr, parent, x_offset + window_x, y_offset + window_y);

Comment on attachment 167995 [details] [review]
Replace GDK drawing API with cairo

Attachment 167995 [details] pushed as 08cfdcd - Replace GDK drawing API with cairo

Pushed with a change to fix the issue pointed out in my comment - thanks Owen for figuring it out!

Attachment 168046 [details] pushed as 0839c10 - Use gdk_window_get_background_pattern() to clear the background

Created attachment 168268 [details] [review]
Fix build against gtk+ older than 2.21.6.

The gdk_window_get_background_pattern() function copied from GDK and
introduced in commit 0839c10 has a small syntax error and uses a private
API (_gdk_drawable_ref_cairo_surface()). This patch imports the missing
API and fixes the syntax error.

Review of attachment 168268 [details] [review]:

Patch looks good.

Comment on attachment 168268 [details] [review]
Fix build against gtk+ older than 2.21.6.

Patch pushed as 4544fe7571b4ed3e1a5e20b3013f44ff4e04e6f9