Created attachment 136065 [details] [review]
Port to Mutter to Clutter 1.0 API

I was going to try to review this, but I really have no clue, so I'll just complain about the new APIs some instead:

+      cogl_material_set_layer_combine (priv->material, 1,
+                                      "RGB = REPLACE (PREVIOUS)"
+                                      "A = MODULATE (PREVIOUS, TEXTURE)",
+                                      NULL);

this really looks like you forgot a ";" or "\n" after the first ")", although it is apparently correct according to the docs...

   gdouble  scale_x    = 1.0;
   gdouble  scale_y    = 1.0;
-  gint     anchor_x   = 0;
-  gint     anchor_y   = 0;
+  gfloat   anchor_x   = 0;
+  gfloat   anchor_y   = 0;

doubles for scale, floats for position? Nice! (On a more constructive note, it would be better with 0.0 instead of 0.)

Created attachment 136130 [details] [review]
Moblin clutter 1.0 patch

We have done this internally for moblin alongside the 1.0-integration branch; the patch differs from Owen's at couple of places.

Created attachment 136131 [details] [review]
Moblin clutter 1.0 patch

Sorry; attached the wrong patch.

Comparing the two patches:

 * Your patch uses NO_SLICING in:


         priv->mask_texture = cogl_texture_new_from_data (tex_width, tex_height,
-                                                         -1, FALSE,
+                                                         COGL_TEXTURE_NO_SLICING,
                                                          COGL_PIXEL_FORMAT_A_8,
                                                          COGL_PIXEL_FORMAT_ANY,
                                                          tex_width,

   Using NO_SLICING is a better literal translation of the -1, but:

   - If using texture_from_pixmap and NPOT textures, isn't relevant. No slicing will be when NPOT textures are available
   - If using texture_from_pixmap and texture_rectangle, isn't relevant. This code path isn't used.
   - If falling back to ClutterX11TexturePixmap, then the window data texture will be created without the NO_SLICING flag. So, if slicing would be used, we're already slicing, and there is no point in forcing this unsliced. (Cogl can't handle multitexturing sliced textures, but it *could* reasonably easily if they are sliced identically.)

  But I guess that's an unrelated change, so probably should be separated out from this patch :-)

 - #if 0'ing the depth=24 workaround in your patch is clearly unrelated and shouldn't be part of this patch

+      /* Replace the RGB from layer 1 with the RGB from layer 0 and
+         modulate the alpha in layer 1 with the alpha from the
+         layer 0 */
+      if (!cogl_material_set_layer_combine (priv->material, 1,
+                                            "RGB = REPLACE (PREVIOUS)\n"
+                                            "A = MODULATE (PREVIOUS, TEXTURE)",
+                                            &error))
+        {
+          g_warning ("%s", error->message);
+          g_clear_error (&error);
+        }
 
 - As compared to this. I removed the comment because I thought it added nothing informative and was just a restating of the API call.

 - I didn't pass in a GError and warn, because I object to having to do that as a really awkward API; possibly if a NULL GError is passed and an error occurs COGL should g_warning() by default. (There's some GLib API that does that, I can't think of which.)

-  mutter_plugin_query_screen_size (plugin,
-					      &screen_width,
-					      &screen_height);
+  mutter_plugin_query_screen_size (plugin, &screen_width, &screen_height);

 - This reformatting in your patch is basically stray.

-          gint x, y;
-          guint w, h;
+          gfloat x, y;
+          gfloat w, h;
 
           clutter_actor_get_position (window, &x, &y);
           clutter_actor_get_size (window, &w, &h);

 - This whole section of code was unused, so I removed it in my patch instead of porting it.

In my patch:

-  gint     anchor_x   = 0;
-  gint     anchor_y   = 0;
+  gfloat   anchor_x   = 0;
+  gfloat   anchor_y   = 0;

With the ints here, you get implicit integer => float conversion when clutter_actor_move_anchor_point() is called, so 42.99999 will end up as 42. Using floats avoids this.

===

OK, so unless you have a different opinion what I'll do is:

 - Go with my patch, but with the NO_SLICING flag
 - Prepare a separate patch that removes NO_SLICING
 - File a bug against Cogl to warn on blend-string errors when a NULL GError is passed, and see what Bob thinks of that.

Go for it.

Patch pushed. Slicing patch is bug 585151.

Patch to make Cogl warn useful is http://bugzilla.openedhand.com/show_bug.cgi?id=1642.