Created attachment 216984 [details] [review]
Patch v1

Review of attachment 216984 [details] [review]:

the patch is missing an update to clutter.symbols, as well as updated sections inside the API reference.

::: clutter/clutter-rotate-action.c
@@ +78,3 @@
+  clutter_gesture_action_get_motion_coords (action, 0, &p1[0], &p1[1]);
+  clutter_gesture_action_get_motion_coords (action, 1, &p2[0], &p2[1]);
+  g_message ("\tp1=%fx%f p2=%fx%f", p1[0], p1[1], p2[0], p2[1]);

left-over g_message()

@@ +89,3 @@
+  priv->initial_rotation = clutter_actor_get_rotation (actor, CLUTTER_Z_AXIS, NULL, NULL, NULL);
+
+  g_message ("rotate begin! %fx%f", priv->initial_vector[0], priv->initial_vector[1]);

left-over g_message()

@@ +95,3 @@
+
+static gboolean
+clutter_rotate_gesture_progress (ClutterGestureAction *action,

by looking at the API, gesture_progress() should emit the ::rotate signal, and the default handler should rotate the actor.

@@ +112,3 @@
+      (vector[1] == priv->initial_vector[1]))
+    {
+      clutter_actor_set_z_rotation_from_gravity (actor,

I guess people will want the RotateAction to respect the current rotation center point of the actor instead of trampling it. in practice, this is almost always correct, and in the future we want to have only a single point for transformations.

@@ +136,3 @@
+        angle = -angle;
+
+      clutter_actor_set_z_rotation_from_gravity (actor,

same as above.

@@ +166,3 @@
+   * Since: 1.12
+   */
+  rotate_signals[ROTATE] =

you never actually emit the ::rotate signal. :-)

::: clutter/clutter-rotate-action.h
@@ +68,3 @@
+ * only private data.
+ *
+ * Since: 1.8

Since: 1.12

@@ +90,3 @@
+};
+
+GType clutter_rotate_action_get_type (void) G_GNUC_CONST;

missing CLUTTER_AVAILABLE_IN_1_12 annotation

Created attachment 217471 [details] [review]
patch v2

Review of attachment 217471 [details] [review]:

looks generally good to me.

clutter.symbols needs to be updated, as well as the API reference bits.

it would be awesome to have an example/interactive test, but that can land later.

::: clutter/clutter-rotate-action.c
@@ +194,3 @@
+   * Since: 1.12
+   */
+  rotate_signals[ROTATE] =

one of the hard lessons of ClutterDragAction is that if an action has a signal with a default behaviour, people will want to override it to do their own thing. in theory, you could connect to the ::gesture-progress and return FALSE, but the angle is computed inside the ::gesture-progress closure, and since ::gesture-progress is RUN_LAST, there's no way to access the angle before the closure is called - unless you use g_signal_connect_after(), but in that case ::rotate will have already been emitted.

ClutterRotateAction::rotate should have a boolean return value, and if any handler returns FALSE, the signal emission chain will be stopped.

::: clutter/clutter-rotate-action.h
@@ +51,3 @@
+ * only private data and should be accessed using the provided API
+ *
+ * Since: 1.8

Since: 1.12

@@ +68,3 @@
+ * only private data.
+ *
+ * Since: 1.8

Since: 1.12.

@@ +90,3 @@
+};
+
+GType clutter_rotate_action_get_type (void) G_GNUC_CONST;

missing CLUTTER_AVAILABLE_IN_1_12 annotation.

@@ +92,3 @@
+GType clutter_rotate_action_get_type (void) G_GNUC_CONST;
+
+ClutterAction * clutter_rotate_action_new        (void);

missing CLUTTER_AVAILABLE_IN_1_12 annotation.

Created attachment 218956 [details] [review]
patch v3

Review of attachment 218956 [details] [review]:

looks good to me; it can go in with minor modifications.

::: clutter/clutter-rotate-action.c
@@ +53,3 @@
+  gdouble initial_rotation;
+
+  int threshold;

this member is not used anywhere.

@@ +73,3 @@
+                                   gdouble              angle)
+{
+  g_object_set (G_OBJECT (actor),

you can call clutter_actor_set_rotation_angle(actor, CLUTTER_Z_AXIS, angle) here.

@@ +98,3 @@
+          priv->initial_vector[1] * priv->initial_vector[1]);
+
+  priv->initial_rotation = clutter_actor_get_rotation (actor, CLUTTER_Z_AXIS,

this should be clutter_actor_get_rotation_angle(), given that get_rotation() has been deprecated.

@@ +198,3 @@
+   *
+   * The ::rotate signal is emitted when a rotate gesture is recognized on the
+   * attached actor.

you should add a documentation note that says that the ::rotate signal will be emitted on ::gesture-cancel as well.

@@ +200,3 @@
+   * attached actor.
+   *
+   * Since: 1.12

missing "Return value" annotation.

@@ +207,3 @@
+                  G_SIGNAL_RUN_LAST,
+                  G_STRUCT_OFFSET (ClutterRotateActionClass, rotate),
+                  _clutter_boolean_handled_accumulator, NULL,

the boolean_handled accumulator will stop the emission if TRUE is returned, and continue if FALSE is returned instead. GestureAction, DragAction, and ClickAction will continue their signal emission unless FALSE is returned; RotateAction should follow the other action classes.

we should probably have a _clutter_boolean_continue_accumulator(), similar to _clutter_boolean_handled_accumulator(), that just stops the emission if FALSE is returned; for the time being, you can copy the accumulator from DragAction, GestureAction, or ClickAction.

landed with the modifications suggested.