Created attachment 189075 [details] [review]
patch 1

(In reply to comment #0)
> The tests don't pass on my machines because ClutterShaderEffect causes
> artifacts on them (i965 and MEGD).

Tests pass now after updating the machine with i965 to Fedora 15.

Created attachment 189304 [details] [review]
Rename ClutterEffect::run to ClutterEffect::paint

In preparation for adding ClutterEffect::pick

Created attachment 189305 [details] [review]
Give a chance to effects for running when picking

Some effects can change the actor's shape and position, so they need
to run when picking.

Created attachment 189330 [details] [review]
Give a chance to effects for running when picking

Some effects can change the actor's shape and position, so they need
to run when picking.

Invalidates the cached texture in ClutterOffscreenEffect if the picking
mode changes.

Review of attachment 189330 [details] [review]:

initial review; I'll let Neil look at the propagated_one_redraw flag issue

::: clutter/clutter-actor.c
@@ +2858,3 @@
+     applications to notify when the value of the
+     has_overlaps virtual changes. */
+  add_or_remove_flatten_effect (self);

add_or_remove_flatten_effect() should only be executed when painting - we're picking actors at full opacity, so we shouldn't use the flatten effect for the opacity.

Created attachment 189409 [details] [review]
Give a chance to effects for running when picking

Some effects can change the actor's shape and position, so they need
to run when picking.

Review of attachment 189409 [details] [review]:

I think this is generally correct and what we discussed.

::: clutter/clutter-effect.h
@@ +102,2 @@
   /*< private >*/
   void (* _clutter_effect3) (void);

you need to remove the corresponding padding from the vtable, otherwise it's an ABI break.

Created attachment 189412 [details] [review]
Give a chance to effects for running when picking

Some effects can change the actor's shape and position, so they need
to run when picking.

Created attachment 189820 [details] [review]
Suggested changes to be squashed into the patch

The patches look good to me but I just have a few comments. I don't think we should be clearing the priv->propagated_one_redraw flag when the actor is picked because then if a pick is performed between two paints then it will assume the actor is not dirty. We don't really have a mechanism to determine when a 'pick' is dirty so I think we should just always pass the dirty flag to the effect.

I don't see why ClutterOffscreenEffect needs to change in the second patch. It was changed to check what the pick mode is in pre_paint and use this value to determine whether to use the cached image in the paint() method. I don't think this makes sense because the pre_paint and paint() methods won't get called during a pick but it wouldn't help anyway because pre_paint is called after the paint() method.

I'm attaching a diff with the suggested changes that I think we should squash into Tomeu's patch.

Created attachment 189829 [details] [review]
effect: Rename RunFlags to PaintFlags

Since run() was replaced and both paint() and pick() use the
enumeration.

pushed the patches, and squashed Neil's patch on top of Tomeu's.

thanks!