Created attachment 170694 [details] [review]
ui: Clip the region once, not every rectangle manually

Created attachment 170695 [details] [review]
Make clip_to_screen() actually clip to the screen

Created attachment 170696 [details] [review]
Remove workaround now that clip_to_screen() works properly

Created attachment 170697 [details] [review]
Remove client area from region in expose event

Created attachment 170698 [details] [review]
Remove NULL checks

This function actually handles NULL. (And I'm going to pass some NULLs
soon).

Created attachment 170699 [details] [review]
Move begin_paint() handling to expose handler

This way, we can remove the special casing in
meta_frames_paint_to_drawable().

Created attachment 170700 [details] [review]
Remove meta_ui_get_pixbuf_from_pixmap() function

Created attachment 170701 [details] [review]
Remove MetaArea widget

It's unused.

Created attachment 170702 [details] [review]
Remove unused code

Created attachment 170703 [details] [review]
Simplify code: Use cairo_paint() to paint the whole pixmap

Also, gdk_drawable_get_size() is going away soon

Created attachment 170704 [details] [review]
Do not create pixmaps when there's nothing to draw

Created attachment 170705 [details] [review]
Change the cached rectangle ares to a GdkRectangle

Simplifies the code as the rectangle is mainly interacting with GDK
APIs.

Created attachment 170706 [details] [review]
Simplify cached area subtraction code

Because we store the affected rectangle in the piece, we can just
subtract it and don't need any complicated computations.

Review of attachment 170694 [details] [review]:

Looks good

Review of attachment 170698 [details] [review]:

Good to commit except subject line which is undescriptive - nothing in the subject line or body mentions where you are making changes

 meta_frame_layout_get_borders: remove NULL checks on parameters

Or something.

Review of attachment 170701 [details] [review]:

Good to commit

Review of attachment 170702 [details] [review]:

Fine to commit, except again the commit message needs to be something that isn't 100% generic

Review of attachment 170703 [details] [review]:

Fine.

Review of attachment 170704 [details] [review]:

Subject: frame.c: Do not create pixmaps when there's nothing to draw

And in body:

 'Take advantage of existing handling for CachedFramePiece.piece == NULL to avoid
  generating pixmaps when width/height are 0'

(or something like that). And then in an appropriate place in populate_cache() add
a:

 /* Returns NULL for 0 width/height pieces, but returns NULL cheaply so we
  * don't need to cache the NULL return */

(or something like that). Since the patch is basically conceptually wrong and just
works out OK, it needs an explanatory comment.

Review of attachment 170705 [details] [review]:

Looks good

Yeah, I suspect it'd be best if I did another patch that unmessed the populate_cache() function and didn't try to populate an already existing cache. Shouldn't be too hard to do.

Review of attachment 170706 [details] [review]:

Looks fine, would prefer a 'frames.c: ' on the Subject

Review of attachment 170700 [details] [review]:

Sure

Review of attachment 170699 [details] [review]:

Sure, except for the below. Add to body of the commit message:

 Since the setup in meta_frames_paint_to_drawable() is relatively cheap,
 doing it once per rectangle in the expose area should be fine.

::: src/ui/frames.c
@@ +2364,3 @@
+      area_region = meta_region_new_from_rectangle (&areas[i]);
+
+      gdk_window_begin_paint_rect (event->window, &areas[i]);

Use gdk_window_begin_paint_region() to avoid double rect => region conversion - MetaRegion is always compatible with the signature of that.

Review of attachment 170696 [details] [review]:

Explanation for the border seems to be:

 Havoc Pennington   [metacity developer]      2007-01-23 14:53:00 UTC 

 One possible fix would be to clip the painting to the screen extents (there is
 a race condition here - the window could move in relation to the screen while
 drawing - maybe one fix for that would be to clip to a bit larger area than the
 screen, and skip the clipping for windows smaller than the screen, so only
 large windows that get moved a lot would repaint incorrectly)

I don't understand the race condition though - the position we have for a window
will be more up to date than any expose events and in fact is synchronoously
in sync with the painting screen (since we are the window manager). So, if according
to the current position, something is offscreen, it's offscreen and doesn't need to
be painted.

And the clipping code is completely broken, as far as I can see - it clips the
region in window coordinates against the screen in absolute coordinates.

<light bulb dawns>

 https://bugzilla.redhat.com/show_bug.cgi?id=557402
 https://bugzilla.gnome.org/show_bug.cgi?id=538438

So I think it's fine to go and remove this code. Needs a commit message that
actually describes what is going on.

Review of attachment 170697 [details] [review]:

Looks OK. (Not convinced that this is really independent from "Move begin_paint() handling to expose handler" - without that patch, this would just be unmotivated random code motion.)

Review of attachment 170695 [details] [review]:

Looks good to me (frames.c: in Subject line)

Created attachment 170747 [details] [review]
Remove MetaImageWindow

The code isn't used anymore.

Created attachment 170748 [details] [review]
ui: gtk_widget_show() the MetaFrames object

The widget needs to be visible and mapped for GTK3 to deliver expose
events to the widget. This is achieved by making the map function a
no-op and calling gtk_widget_show() instead of just calling
gtk_widget_realize().
Apart from making GTK think the widget is drawable, the effect is the
same.

Created attachment 170749 [details] [review]
Make Mutter work with GTK3 after the latest changes

With the newest changes to GTK3, some things were changed. This patch
introduces an ugly macro compatibility layer in drawcompat.[ch] that
ensures compilation against both GTK2 and GTK3.
The following changes were necessary:

- MetaPixmap
MetaPixmap is the replacement for GdkPixmap. It uses GdkPixmap on GTK2
and cairo_surface_t on GTK3 and is just simple defines.

- MetaDrawContext
MetaDrawContext encapsulates the 2nd argument passed to the
expose_event/draw signal from GtkWidget. A small API is layered on top
that fulfills all the current needs. In particular, it is used to create
a cairo context.

- switching theme drawing to a cairo_t
Instead of passing a drawable and a clip area, the code now passes a
cairo_t around. The cairo_t must not be scaled, rotated or skewed and be
created from a MetaDrawContext though (see next point), so the cairo_t
is pretty much only a tool to cary around the drawable and the clip
area.

- gtk_paint_foo()
the GtkStyle paint functions now take a cairo_t instead of a drawable
and clip area. The GTK2 code implements this by extracting the
GdkExposeEvent/MetaDrawContext and the clip area from the cairo_t and
passing those to the old-style paint functions.

(In reply to comment #24)
> ::: src/ui/frames.c
> @@ +2364,3 @@
> +      area_region = meta_region_new_from_rectangle (&areas[i]);
> +
> +      gdk_window_begin_paint_rect (event->window, &areas[i]);
> 
> Use gdk_window_begin_paint_region() to avoid double rect => region conversion -
> MetaRegion is always compatible with the signature of that.

I used begin_paint_rect() because in the large refactoring I used rectangles, so I could gdk_cairo_rectangle(); cairo_clip(); in the GTK3 code. So it makes the large patch conform more to the code I'm introducing here.

Review of attachment 170747 [details] [review]:

Sure.

Review of attachment 170748 [details] [review]:

Looks good except for comments

::: src/ui/frames.c
@@ +680,3 @@
 {
+  /* We override the parent map function to a no-op because we don't
+   * want to actually whow the GDK window. We only use the GDK window

type 'whow'

Not sure what you mean by "only use the GDK window" - theme changes are notified using GtkWidget facilities. Do you mean "We only derive from GtkWidget ..." ?

@@ +683,3 @@
+   * so it notifies us about theme changes and other useful things.
+   */
+  gtk_widget_set_mapped (widget, TRUE);

This probably needs an entry in src/gdk-compat.h since it is 'Since 2.20' (probably could up the GTK+ requirement at this point to 2.20 if the list of things we have to compat define gets too long.)

It's a little scary to bypass gtk_window_map() since it does a *lot* of stuff other than map the window, but I can't find anything else in there that matters here, so OK.

::: src/ui/ui.c
@@ +129,3 @@
   g_assert (xdisplay == GDK_DISPLAY_XDISPLAY (gdk_display_get_default ()));
   ui->frames = meta_frames_new (XScreenNumberOfScreen (screen));
+  gtk_widget_show (GTK_WIDGET (ui->frames));

Needs a comment, maybe:

 /* meta_frames_map() has been hacked so that showing a meta_frames doesn't
  * actually show anything. But we need the VISIBLE flag set so events
  * get delivered */

Review of attachment 170749 [details] [review]:

This is where I usually end up with splitty approaches to Git patches - I split about 20% off as nice little chunks for pre-cleanup, then I have a huge piece that actually does something that can't be split up :-)

So, two main things I see here - first the handling of ::expose-event vs ::draw is a bit too clever for me. Described in detail below.

The second issue is that, as far as I can see, you've changed the public header theme.h so that it takes cairo_t, but for GTK+-2.0, it has to be a "special" cairo_t with a GdkEventExpose tagged on it. And there's not even a way of creating such a cairo_t in anything installed publicly.

I'm perfectly happy addressing that as follows:

 - Document the problem in the missing block comment at the top of drawcompat.h (you do a pretty good job of describing things in your commit message, but commit messages aren't available reading the code without a bit of digging so information needed to understand what's going on should generally not be hidden in commit messages.... historical rationales, alternate approaches, etc, are fine in commit messages, or even bugzilla, but "how do I use this" should be in the code)

 - g_error more informatively than g_assert(event != NULL);

 - Don't install libmutter-private for GTK+-2.0 builds. GTK+ 2 builds are for:

    Meego: As I understand it, they've hacked out Metacity themes and are putting clutter actors around windows
    People using Mutter for alternate interfaces to GNOME 2: GNOME 2 links to libmetacity-private not libmutter-private (so does GNOME 3, but might change at some point)

::: src/gdk-compat.h
@@ +27,3 @@
+gdk_drawable_get_size (GdkWindow *window,
+                       int       *width,
+                       int       *height)

All compat needs to be single-directional. Emulating both ways is too confusing - so you need compat implementations of gdk_window_get_width()/height()

::: src/ui/drawcompat.c
@@ +28,3 @@
+#if GTK_CHECK_VERSION (2, 90, 0)
+
+#else /* GTK_CHECK_VERSION */

as noted elsewhere, please use /* GTK+ < 2.90.0 */ so that the #else is interpretable in isolation

@@ +48,3 @@
+      cairo_translate (cr,
+                       allocation.x,
+                       allocation.y);

Unless you've changed the meaning of allocation.x/.y in GTK+ 3, this is wrong. (it's right only for no-window widgets)

@@ +102,3 @@
+}
+
+void meta_paint_vline (GtkStyle           *style,

Bad indentation, and throughout after this - void needs to be on a separate line

::: src/ui/drawcompat.h
@@ +34,3 @@
+
+#define DRAW_EVENT draw
+#define DRAW_EVENT_NAME "draw"

I'm of mixed feelings here. On one hand, what you've done by exploiting the fact that ::expose-event and ::draw have similar signatures keeps things compact and very clean looking. (I'm not completely sure I agree with the boolean return from ::draw - the boolean return from ::expose-event is a frequent trap where people inadvertently return TRUE and prevent people from "decorating" on top of the standard drawing, but anyways, a different topic.) But the cleanliness also makes it pretty unclear what is going on looking locally at the code. Someone has to look at the code, scratch their head, read draw-compat.h, understand it, then say "Oh, that's what they are doing here". My preference is really to make the code messier and do something like:

#ifdef USE_GTK2_DRAWING
gboolean
meta_foo_expose_event (GtkWidget *widget, MetaDrawContext *context)
/* MetaDrawContext == GdkEventExpose */
#else
gboolean
meta_foo_draw (GtkWidget *widget, MetaDrawContext *context)
/* MetaDrawContext == cairo_t */
#endif

That gives a strong hint to someone reading a local portion of the code going on - and if they don't actually have to do anything complex the MetaDrawContext they'll have a sufficient picture of the situation to go on and do what they actually needed to do in the code. I know it's annoying to have to do that not just for the definitions, but for the declarations and for the places where the vfuncs are assigned, but we're talking 10 or so places total.

@@ +37,3 @@
+
+#define meta_pixmap_cairo_create(pixmap) cairo_create (pixmap)
+#define meta_pixmap_create(window, w, h) \

meta_pixmap_create => construct a MetaPixmap

Implies:

 meta_pixmap_cairo_create => construct a MetaPixmapCairo

Suggest 'meta_pixmap_new' for the first - after all, you called it like GdkPixmap not cairo_surface_t, so use GDK/Metacity conventions for constructors.

@@ +41,3 @@
+#define meta_pixmap_destroy(pixmap) cairo_surface_destroy (pixmap)
+#define meta_cairo_set_source_pixmap(cr, pixmap, x, y) \
+  cairo_set_source_surface (cr, pixmap, x, y)

Mixture of line breaking and non-line breaking isn't readable. We aren't on VT100's, just keep each #define on one line

@@ +52,3 @@
+}
+static inline Window
+meta_draw_get_xid(cairo_t *cr)

This is a method on "MetaDrawContext", so 'meta_draw_context_get_xid - reduces general namespace pollution as well.

@@ +63,3 @@
+
+#define meta_draw_create(ctx, widget) cairo_reference (ctx)
+#define meta_draw_begin_paint_rect(ctx, rect) G_STMT_START{ \

If you are going to use static inlines for some things, then use them here too.

@@ +92,3 @@
+#define meta_paint_layout gtk_paint_layout
+
+#else /* GTK_CHECK_VERSION */

This isn't a useful comment on the else - better to have /* GTK+ < 2.90.0 */ or something

::: src/ui/frames.c
@@ +2367,2 @@
 static void
+meta_frames_paint_to_drawable (MetaFrames      *frames,

This function needs a rename to go along with the signature change (meta_frames_paint() would be fine if it's not used elsewhere)

::: src/ui/metaaccellabel.c
@@ +286,3 @@
 	  layout = gtk_widget_create_pango_layout (widget, accel_label->accel_string);
 
+          cr = meta_draw_create (ctx, widget);

You leak this cr

::: src/ui/tabpopup.c
@@ +79,3 @@
+outline_window_draw (GtkWidget       *widget,
+                     MetaDrawContext *ctx,
+                     gpointer         data)

This creates a cairo_t internally, should be able to use the one passed in

@@ +706,3 @@
 static gboolean
+meta_select_image_draw (GtkWidget       *widget,
+                        MetaDrawContext *ctx)

As does this

::: src/ui/theme.c
@@ +3861,3 @@
         rheight = parse_size_unchecked (op->data.tile.height, env);
 
+        cairo_save (cr);

Don't see why this save/restore pair is necessary

@@ +3865,2 @@
+        cairo_rectangle (cr, rx, ry, rwidth, rheight);
+        cairo_clip (cr);

I think the completely-clipped-out optimization you've removed here is probably significant - it's never worth doing:

 if (!<completely clipped>) 
     cairo_rectangle (cr, ...);
     cairo_fill();
 }

Because cairo should take care of it itself, but there's more going on inside here - parsing, drawing a list of operations, etc.

@@ +4010,1 @@
+  cairo_save (cr);

You have a function takes a cr as an argument, does a save/restore around the body of the function, but *modifies* the cr that is passed in. That's going to confuse someone! Probably best if functions in the publically installed theme.h leave their cairo_t arguments as they found them and don't require the caller to save/restore.

@@ +4027,1 @@
+          cairo_save (cr);

Maybe you meant to move this save() before the cairo_rectangle/clip pair?

@@ -4595,1 @@
-              if (combined_clip.width > 0 && combined_clip.height > 0)

Same comment about removing the "completely out of the scene" check here. (Would be handy if cairo had a predicate for "is this rectangle completely clipped away")

::: src/ui/ui.c
@@ +171,3 @@
   GdkWindow *window;
   GdkVisual *visual;
+  //GdkColormap *cmap = gdk_screen_get_default_colormap (screen);

This and more in-progress leftovers in this file. Not sure where you are going with it.

@@ +359,2 @@
 GdkPixbuf*
 meta_gdk_pixbuf_get_from_window (GdkPixbuf   *dest,

Don't really trust the colormap handling in your replacement in the gtk2 version, but this function is unused right? Any reason you didn't just remove it? I think this might have been used by the Xlib compositor for some ugly effects.

(In reply to comment #34)
> My preference is really to make the code messier and do something like:
> 
> #ifdef USE_GTK2_DRAWING
> gboolean
> meta_foo_expose_event (GtkWidget *widget, MetaDrawContext *context)
> /* MetaDrawContext == GdkEventExpose */
> #else
> gboolean
> meta_foo_draw (GtkWidget *widget, MetaDrawContext *context)
> /* MetaDrawContext == cairo_t */
> #endif
> 
> That gives a strong hint to someone reading a local portion of the code going
> on - and if they don't actually have to do anything complex the MetaDrawContext
> they'll have a sufficient picture of the situation to go on and do what they
> actually needed to do in the code. I know it's annoying to have to do that not
> just for the definitions, but for the declarations and for the places where the
> vfuncs are assigned, but we're talking 10 or so places total.

Quick clarification here - by "do that not just for the definitions, but for the declarations and the places where the vfuncs are assigned" I meant the #ifdefs, not the comments.

(In reply to comment #34)
>  - Don't install libmutter-private for GTK+-2.0 builds. GTK+ 2 builds are for:
> 
>     Meego: As I understand it, they've hacked out Metacity themes and are
> putting clutter actors around windows

That is not the case, MeeGo uses normal metacity themes for theming applications, and we do need libmutter-private installed.

(In reply to comment #36)
> (In reply to comment #34)
> >  - Don't install libmutter-private for GTK+-2.0 builds. GTK+ 2 builds are for:
> > 
> >     Meego: As I understand it, they've hacked out Metacity themes and are
> > putting clutter actors around windows
> 
> That is not the case, MeeGo uses normal metacity themes for theming
> applications, and we do need libmutter-private installed.

OK, I must have misunderstood a plan or experiment for what you were doing.

 - You are using libmutter-private for a theme selector?
 - Are you using functions like meta_frame_style_draw() that take a GdkDrawable as an argument?

Question: Could we make Mutter use GtkAccelLabel instead of its
own widget? Or is the accelerator handling too different in Mutter and GTK?
That'd get rid of a rather large #ifdef case for expose events.

For the other widgets, I'd say ifdeffing is a valid use case.

But making theme drawing work without a cairo_t in GTK 3 is pretty much impossible I guess.

I might be able to fix the special requirements for GTK2 by creating a pixmap and gtk_paint_foo() to it and then blit that onto the cairo_t, so the special case wouldn't be needed, but that'd still require different APIs.

(In reply to comment #38)
> Question: Could we make Mutter use GtkAccelLabel instead of its
> own widget? Or is the accelerator handling too different in Mutter and GTK?
> That'd get rid of a rather large #ifdef case for expose events.

GtkAccelLabel hard-codes the GTK+ accelerator system:

void       gtk_accel_label_set_accel_widget  (GtkAccelLabel *accel_label,
                                              GtkWidget     *accel_widget);

Etc. There's very little commonality between what Mutter is doing with passive grabs and GTK+ accelerator handling. Without actually trying to figure out the massive hacks that would be required to try and make it to work, I'm pretty sure it would be a bad idea.

> For the other widgets, I'd say ifdeffing is a valid use case.
> 
> But making theme drawing work without a cairo_t in GTK 3 is pretty much
> impossible I guess.
> 
> I might be able to fix the special requirements for GTK2 by creating a pixmap
> and gtk_paint_foo() to it and then blit that onto the cairo_t, so the special
> case wouldn't be needed, but that'd still require different APIs.

I think we need to wait and get more details from Tomas about what they are doing. One possible approach would be to have compatibility wrappers for GTK+ 2 users that export the old API and then create the magic cairo_t internally and call the real implementation. Another approach would be to export a meta_draw_context_new (...); function in the GTK+ 2. case.

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #34)
> > >  - Don't install libmutter-private for GTK+-2.0 builds. GTK+ 2 builds are for:
> > > 
> > >     Meego: As I understand it, they've hacked out Metacity themes and are
> > > putting clutter actors around windows
> > 
> > That is not the case, MeeGo uses normal metacity themes for theming
> > applications, and we do need libmutter-private installed.
> 
> OK, I must have misunderstood a plan or experiment for what you were doing.
> 
>  - You are using libmutter-private for a theme selector?

We do, but on reflection it does not make much sense to hold onto the gtk2 code paths simply because of MeeGo (I can't see us developing these codepaths for MeeGo much further), so if nobody else is using these, then I would say let's excise it for sake of a cleaner codebase when that is convenient from the Gnome pov.

(In reply to comment #40)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > (In reply to comment #34)
> > > >  - Don't install libmutter-private for GTK+-2.0 builds. GTK+ 2 builds are for:
> > > > 
> > > >     Meego: As I understand it, they've hacked out Metacity themes and are
> > > > putting clutter actors around windows
> > > 
> > > That is not the case, MeeGo uses normal metacity themes for theming
> > > applications, and we do need libmutter-private installed.
> > 
> > OK, I must have misunderstood a plan or experiment for what you were doing.
> > 
> >  - You are using libmutter-private for a theme selector?
> 
> We do, but on reflection it does not make much sense to hold onto the gtk2 code
> paths simply because of MeeGo (I can't see us developing these codepaths for
> MeeGo much further), so if nobody else is using these, then I would say let's
> excise it for sake of a cleaner codebase when that is convenient from the Gnome
> pov.

OK, there's still Ubuntu Unity using a GTK+ 2 build of Mutter. (It's obviously lower priority for me than GNOME 3, but I'm reluctant to rip GTK+ 2.0 support out if I'm then going to get a push to add it back in.)

But if you don't have firm plans for when you'll be using this version of Mutter and what GTK+ version with that, then I don't want to make a lot of extra work to create a usable GTK+ 2 libmutter-private. The approaches in comment 39 could be implemented later. So, I think we should go with my plan in comment 34 for now.

Comment on attachment 170696 [details] [review]
Remove workaround now that clip_to_screen() works properly

Added in your comments and put links to all the bugs into the commit message.

Comment on attachment 170699 [details] [review]
Move begin_paint() handling to expose handler

Committed with the improvements noted in comment 24

Comment on attachment 170704 [details] [review]
Do not create pixmaps when there's nothing to draw

Committed with added comments.

Comment on attachment 170748 [details] [review]
ui: gtk_widget_show() the MetaFrames object

Added comments as per comment 33. gtk_widget_set_mapped() was already existing in gtk-compat.h.

(In reply to comment #30)
> Created an attachment (id=170749) [details] [review]
> Make Mutter work with GTK3 after the latest changes

Looks like tile-preview is missing from the patch?

/me ducks

Created attachment 170985 [details] [review]
ui: Remove unused meta_gdk_pixbuf_get_from_window()

Created attachment 170986 [details] [review]
build: Only install libmutter-private for GTK3 builds

Define INSTALL_LIBMUTTER_PRIVATE with AM_CONDITIONAL and use it to build
an installed or uninstalled libmutter-private.so

Created attachment 170987 [details] [review]
theme: Upgrade to use Cairo for painting (changes API)

This commit is in preparation for the work happening in GTK3, which will
use Cairo for drawing exclusively. So it is necessary to move all
drawing code to Cairo.
For compatibility with older GTK versions, the file gtk3-compat.h
provides a compatibility layer.

The commit changes the API of libmutter-private.

Created attachment 170988 [details] [review]
frames.c: Change meta_frames_paint() to take a cairo_t

Rename meta_frames_paint_to_drawable() to meta_frames_paint() and make
it take a cairo_t as an argument instead of creating the cairo_t itself.

This patch refactors code for GTK3 changes where code needs to handle
cairo_t and not GdkDrawable arguments.

Created attachment 170989 [details] [review]
frames.c: Merge clear_backing() function into its only user

Avoids creating a cairo_t twice. And without the code to create a
cairo_t, there was not a lot left to warrant a separate function.

Created attachment 170990 [details] [review]
Introduce MetaPixmap compatibility wrapper

Similar to the region compatibility shim, we will soon need a
compatibility shim around GdkPixmap/cairo_surface_t. For now, the patch
just introduces the compatibility layer.

This patch also does not include the function
meta_gdk_pixbuf_get_from_pixmap() as that function will need special
treatment in GTK3 anyway.

Created attachment 170991 [details] [review]
ui: Port draw_workspace() to take a cairo_t

I just found a comment that reads:

  /* We make an assumption that gdk_window_new() is going to call
   * XCreateWindow as it's first operation; this seems to be true currently
   * as long as you pass in a colormap.
   */

I'm not sure how important this is, but it doesn't hold true for GTK3 anymore, because you cannot pass in a colormap and GTK3 will create one if it hasn't yet for the given visual. Do we need any API that ensures the first thing gdk_window_new() does is create a window? Or can we lift that requirement? Because it's certainly not guaranteed by GTK...

Created attachment 171022 [details] [review]
Introduce MetaPixmap compatibility wrapper

Similar to the region compatibility shim, we will soon need a
compatibility shim around GdkPixmap/cairo_surface_t. For now, the patch
just introduces the compatibility layer.

This patch also does not include the function
meta_gdk_pixbuf_get_from_pixmap() as that function will need special
treatment in GTK3 anyway.

Created attachment 171023 [details] [review]
frames.c: Make cached_pixels_draw() take a cairo_t

This basically just moves the creation of the cairo_t out of the
function. It is done in preparation for GTK3.

Created attachment 171024 [details] [review]
Fix compilation against latest GTK3 changes

With the newest changes to GTK3, some things were changed. This patch
now uses the features introduced in gtk3-compat.h in previous patches.

This patch also introduces a macro named USE_GTK3 that is used to
differentiate between GTK3 and GTK2. Its main use is differenting
between expose and draw handlers for GtkWidget subclasses.

The draw vs expose handlers question is usually handled by using ifdefs
at the beginning and end to set up/tear down a cairo_t and then use it.

However, when the function is too different and too many ifdefs would be
necessary, two versions of the function are written. This is currently
the case for:
- MetaAccelLabel
- MetaFrames

(In reply to comment #54)
> I just found a comment that reads:
> 
>   /* We make an assumption that gdk_window_new() is going to call
>    * XCreateWindow as it's first operation; this seems to be true currently
>    * as long as you pass in a colormap.
>    */
> 
> I'm not sure how important this is, but it doesn't hold true for GTK3 anymore,
> because you cannot pass in a colormap and GTK3 will create one if it hasn't yet
> for the given visual. Do we need any API that ensures the first thing
> gdk_window_new() does is create a window? Or can we lift that requirement?
> Because it's certainly not guaranteed by GTK...

I told Florian when he was asking about getting the serial in meta_tile_preview_new() that it didn't matter if the request was a few early - but that wasn't 100% right. There's a race condition where things go mildly wrong if we have a too early serial.

 CreateColormap - serial=100, recorded in STACK_OP_ADD,
 Some other app creates a window, CreateWindowEvent has serial 100
 w = CreateWindow - serial=101, CreateWindowEvent has serial 101
 RaiseWindow(w) - serial=102, recorded in STACK_OP_RAISE_ABOVE

 Receive CreateWindowEvent for other app with serial 100
   Discard STACK_OP_ADD because the serial is <= the serial of received events
   Replay remaining recorded requests
   STACK_OP_RAISE_ABOVE produces warning
     "STACK_OP_RAISE_ABOVE: window %#lx not in stack"
   Repaint screen, window is missing because not in stack

 Receive CreateWindowEvent for our window
   Replay remaining recorded requests
   Repaint screen, window is back everything is fine
   
Of course, for your described case, the race occurs at most once in the lifetime of Mutter, and only when some other client is scheduled at exactly that point. So, while gdk_x11_window_get_creation_serial() would be handy, I'd be pretty comfortable ignoring this problem or just adding a:

  (In GTK+ 3 this is not strictly true; in some circumstances GDK will
  create a colormap from gdk_window_new() before creating the window.
  which would cause a race condition and in rare cases, problems,
  but we'd recover -
  see https://bugzilla.gnome.org/show_bug.cgi?id=630203#c55 for details)

(In reply to comment #58)
> Of course, for your described case, the race occurs at most once in the
> lifetime of Mutter, and only when some other client is scheduled at exactly
> that point. So, while gdk_x11_window_get_creation_serial() would be handy
>
I was more thinking about an API like:
Colormap gdk_x11_visual_get_colormap (GdkVisual *visual);
which you could call to ensure the colormap was created already. It's currently a function private to gdk-x11, but wouldn't be too hard to export.
On the other hand, having less APIs exported is always better.

Review of attachment 170985 [details] [review]:

Good

Review of attachment 170986 [details] [review]:

Looks like it is right, if you wanted to try a 'make distcheck' to make sure that everything still gets disted that should get disted, that would be appreciated.

Review of attachment 170986 [details] [review]:

(Though actually, since I'll always be disting with a gtk3 compilation, that's pretty unlikely to break from this patch)

Created attachment 171055 [details] [review]
theme: Upgrade to use Cairo for painting (changes API)

This commit is in preparation for the work happening in GTK3, which will
use Cairo for drawing exclusively. So it is necessary to move all
drawing code to Cairo.
For compatibility with older GTK versions, the file gtk3-compat.h
provides a compatibility layer.

The commit changes the API of libmutter-private.

Created attachment 171057 [details] [review]
Introduce MetaPixmap compatibility wrapper

Similar to the region compatibility shim, we will soon need a
compatibility shim around GdkPixmap/cairo_surface_t. For now, the patch
just introduces the compatibility layer.

This patch also does not include the function
meta_gdk_pixbuf_get_from_pixmap() as that function will need special
treatment in GTK3 anyway.

Created attachment 171058 [details] [review]
Fix compilation against latest GTK3 changes

With the newest changes to GTK3, some things were changed. This patch
now uses the features introduced in gtk3-compat.h in previous patches.

This patch also introduces a macro named USE_GTK3 that is used to
differentiate between GTK3 and GTK2. Its main use is differenting
between expose and draw handlers for GtkWidget subclasses.

The draw vs expose handlers question is usually handled by using ifdefs
at the beginning and end to set up/tear down a cairo_t and then use it.

However, when the function is too different and too many ifdefs would be
necessary, two versions of the function are written. This is currently
the case for:
- MetaAccelLabel
- MetaFrames

Comment on attachment 171024 [details] [review]
Fix compilation against latest GTK3 changes

I forgot to add gtk3-compat.[ch] in those patches, here's the new versions of those. I think the fact that patches now are not in order anymore doesn't matter, they should still apply.

Review of attachment 171055 [details] [review]:

Find this commit a little confusing in where it's going. Commit message should have something like "In this commit the "gtk2" code is used for both gtk2 and gtk3; compatibility with newer versions of gtk3 where different code is needed will be added subsequently."

Once I understood that, mostly made sense to me.

::: src/gtk3-compat.c
@@ +67,3 @@
+
+static GdkWindow *
+extract_event (cairo_t *cr, int *dx, int *dy, GdkRectangle *area)

This doesn't extract an event
Parameters on separate lines

@@ +123,3 @@
+}
+
+void meta_paint_arrow (GtkStyle           *style,

void on the wrong line (and the same elsewhere in this file)

::: src/gtk3-compat.h
@@ +25,3 @@
+#include <gtk/gtk.h>
+
+gboolean      gdk_cairo_get_clip_rectangle (cairo_t            *cr,

This should be handled in gdk-compat.h - it's conceptually different from the rest of the stuff here. If you don't feel comfortable with it as a static online, then add src/gdk-compat.c.

::: src/ui/frames.c
@@ +40,2 @@
 #include "gtk-compat.h"
 #include "gdk-compat.h"

There's obviously a header file naming problem when you look at the above. Suggest 'gdk2-drawing-utils.[ch]"

::: src/ui/theme.c
@@ +4553,1 @@
+              if (op_list)

You are still dropping the completely clipped-check. I need one of three things:

 - An explanation of why it was a stupid idea
 - A comment saying /* This code to check if 'rect' was completely outside of the
   clip region; that's probably a good idea, but it's hard to do with cairo */
 - Restoration of the check

Review of attachment 170988 [details] [review]:

Looks good

Review of attachment 170989 [details] [review]:

Fine

Review of attachment 170991 [details] [review]:

Looks fine

Review of attachment 171023 [details] [review]:

Sure

Review of attachment 171057 [details] [review]:

Sure

Review of attachment 171058 [details] [review]:

I would have been more free with the #ifdefs to avoid extensive duplicated code for the accel label and frames, but it's clearly a matter of judgement, so your approach is fine too.

It was a little hard for me to check the accel label and frames code, so I'm taking that on faith and a quick scan, the rest looks good, except for some details below.

::: src/gtk3-compat.c
@@ +69,3 @@
 
+int
+gdk_window_get_width (GdkWindow *window)

Would like to see these in gdk-compat.h, and not split up compatibility wrappers multiple places. These look fine as static inlines.

::: src/gtk3-compat.h
@@ +55,3 @@
+#define gdk_window_get_screen gdk_drawable_get_screen
+#define gdk_pixbuf_get_from_window(dest, window, src_x, src_y, dest_x, dest_y, width, height) \
+    gdk_pixbuf_get_from_drawable(dest, window, NULL, src_x, src_y, dest_x, dest_y, width, height)

Really don't like defining away a parameter like this - would rather it was a dual define of meta_gdk_pixbuf_get_from_window

::: src/ui/frames.c
@@ +2340,3 @@
+      cairo_clip (cr);
+
+      cairo_push_group (cr);

Not really sure why you are inlining gdk_window_begin_paint_* unless you've axed it in GDK, but OK

::: src/ui/metaaccellabel.c
@@ +249,3 @@
+
+  direction = gtk_widget_get_direction (widget);
+  ac_width = meta_accel_label_get_accel_width (accel_label);

Either you've cut-and-pasted extensively from gtkaccellabel.c (haven't compared the two code bases) or you've made a bunch of irrelevant changes for no apparent reason. If the former, you should include a source acknowledgment in a comment.

Comment on attachment 171055 [details] [review]
theme: Upgrade to use Cairo for painting (changes API)

Committed with the changes you mentioned. I put the all-clipped check back in.

Comment on attachment 171058 [details] [review]
Fix compilation against latest GTK3 changes

> Not really sure why you are inlining gdk_window_begin_paint_* unless you've
> axed it in GDK, but OK
>
I used cairo_push_group() because it was less code and does the same thing under the hood. Otherwise I'd have had to create a new cairo_t for the region that was started to paint and destroy it.
It's also more future-proof because I'm still toying with the idea of moving double-buffering out of GDK and into GTK but I'm not yet 100% sure about how to do it.

Merged everything from GTK to Mutter. The whole jhbuild should keep working.

Created attachment 171193 [details] [review]
frames.c: Clip the client area correctly

The code switched the x and y variables. Oops.

Created attachment 171194 [details] [review]
theme: Get rid of x/y_offset usage when drawing frames

The cairo_t can take care of offsets, so no need to use them.
And since offsets seem to be broken, it's the easiest thing to just
remove them instead of fixing them.

This patch changes API.

Seems I messed some things up in the patch. Here's two more patches that should fix things up hopefully for real now.

Review of attachment 171193 [details] [review]:

Looks right. 

(I was going to ask you to use top/left temporaries on this code on my earlier review, and then decided it didn't matter. Don't know if would have actually helped prevent this error - top/bottom/left/right get ordered in many different ways in different places.)

Review of attachment 171194 [details] [review]:

The changes all look sensible.

Suggest slight wording change to the commit message since "seem to be broken"
read to me as "never worked". Change:

 And since offsets seem to be broken, it's the easiest thing to just
 remove them instead of fixing them.

To:

 The change to passing cairo_t around broke offsets, since passing
 cairo_t makes them unnecessary, remove them rather than fixing them.