Created attachment 221622 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

PanAction is a GestureAction-subclass that implements the panning
concept for scrollable actors. PanAction provides:

• pan-begin and pan-end signals, to mark the event sequence boundaries;

• pan-progress signal, reporting the incremental motion delta to
  user code;

• pan-axis property, to allow constraining the dragging to a specific
  axis.

An interactive test demonstrating its usage with ScrollActor also provided.

Created attachment 221623 [details] [review]
pan-action: add inertial/kinetic events

Port the kinetic scroll logic from MxKineticScrollView to generate
fake progress events with decreasing delta after the user pointer
has been released. PanAction now provides:

• pan-inertial-begin signal to divide the standard ::pan-progress
  events from the ::pan-inertial-progress event sequence;

• pan-inertial-progress signal, analogous to ::pan-progress, to notify
  user code of the decreasing inertial momentum;

• inertial property, to enable or disable the inertial behaviour;

• deceleration property, to customize the rate at which the inertial
  panning will slow down;

• acceleration-factor property, applied to the inertial momentum when
  starting the ::pan-inertial-progress sequence.

The pan-action example has been updated to make use of inertial events.

Review of attachment 221622 [details] [review]:

looks good, except the reimplementation of the GestureAction signals, which I think it's wrong.

::: clutter/clutter-pan-action.c
@@ +272,3 @@
+   * Since: 1.12
+   */
+  pan_signals[PAN_BEGIN] =

I don't think this is necessary at all, and doesn't match the other actions. you can use the gesture-begin signal.

@@ +292,3 @@
+   * Since: 1.12
+   */
+  pan_signals[PAN_PROGRESS] =

same as above: this can be gesture-progress.

if you want to have a signal emission, I'd use something like ::pan-update, though I'd probably just use a property with the gesture distance, and then ask users to connect to notify::pan-distance.

@@ +314,3 @@
+   * Since: 1.12
+   */
+  pan_signals[PAN_END] =

and, again, this can be gesture-end.

Review of attachment 221623 [details] [review]:

this is looking a bit more complicated than I ever intended.

I'd expect users of this gesture to connect to ::gesture-progress and ask for the distance and velocity of the panning, or use a notification on a property to translate the actor. to be fair, tough, I'd make the actor associated with the gesture simply move on ::gesture-progress (with a cumulative velocity factor for multiple pan gestures) and then start an interpolation on ::gesture-end either using a deceleration factor or using an easing mode with a computed end value depending on the velocity.

::: clutter/clutter-pan-action.c
@@ +290,3 @@
+
+  /* Get time delta */
+  g_get_current_time (&release_time);

I'd use the event time here, not the wallclock; the wallclock also can go backwards, for instance.

@@ +295,3 @@
+  priv->last_motion ++;
+  x_origin = y_origin = 0;
+  motion_time = (GTimeVal){ 0, 0 };

this is a GCC extension; also, don't use GTimeVal for time keeping: use gint64.

@@ +720,3 @@
+   * Since: 1.12
+   */
+  pan_signals[PAN_INERTIAL_PROGRESS] =

again, these are far too many signals; you can use a boolean flag to distinguish between an inertial pan gesture and a regular one.

(In reply to comment #3)

> +  pan_signals[PAN_BEGIN] =
> 
> I don't think this is necessary at all, and doesn't match the other actions.
> you can use the gesture-begin signal.

I see. My only concern is that, as I envisioned it, the ::pan-end signal should be emitted after the inertial phase (if present), extending the duration of the gesture even after the user has released the pointer.

To be clearer, the current flow is something like that (with the inertial behaviour enabled):

::gesture-begin → ::pan-begin
::gesture-progress → ::pan-progress
::gesture-end → ::pan-inertial-begin
::new_frame (from the deceleration timeline) → ::pan-inertial-progress
::stopped (from the deceleration timeline) → ::pan-end

Also, I deemed more useful a ::pan-inertial-begin signal vs. a boolean on ::pan-progress because otherwise you need to wait for the next frame to know that the real panning has ended: eg. on reaching the upper limit of a elastic scrollable view you just ignore inertial event and snap the scrollable content as soon as you're sure the user has released the pointer, instead of waiting for the signal generated on ::new_frame to see that the boolean has changed (it also reduce a bit of state-tracking for the user, which I think is a good thing).

I'm not sure what you are proposing instead: can you please elaborate a bit on your proposed solution?
For the non-inertial case using the ::gesture-* should be straightforward, but what should I do in the inertial case? Should I stop the emission of ::gesture-end (with g_signal_stop_emission_by_name()) and generate ::gesture-progress events? Should I update the _gesture_motion_coords() with fake values or should I provide a PanAction-specific method (the 'pan-distance' property)?

(In reply to comment #4)
> this is looking a bit more complicated than I ever intended.

:(
 
> I'd expect users of this gesture to connect to ::gesture-progress and ask for
> the distance and velocity of the panning, or use a notification on a property
> to translate the actor. to be fair, tough, I'd make the actor associated with
> the gesture simply move on ::gesture-progress (with a cumulative velocity
> factor for multiple pan gestures) and then start an interpolation on
> ::gesture-end either using a deceleration factor or using an easing mode with a
> computed end value depending on the velocity.

So you'd move the actor on ::gesture-progress and then just keep moving it after ::gesture-end? What if I want to override the action (maybe because I already have a scrollable container)?

> ::: clutter/clutter-pan-action.c
> @@ +290,3 @@
> +
> +  /* Get time delta */
> +  g_get_current_time (&release_time);
> 
> I'd use the event time here, not the wallclock; the wallclock also can go
> backwards, for instance.

Yup, absolutely. Got it from MxKineticScrollView but I've replaced it with a gint64 and g_get_monotonic_time() (in the ::gesture-progress/end handler I have no event to get the time from).

> @@ +295,3 @@
> +  priv->last_motion ++;
> +  x_origin = y_origin = 0;
> +  motion_time = (GTimeVal){ 0, 0 };
> 
> this is a GCC extension; also, don't use GTimeVal for time keeping: use gint64.

Done.

> @@ +720,3 @@
> +   * Since: 1.12
> +   */
> +  pan_signals[PAN_INERTIAL_PROGRESS] =
> 
> again, these are far too many signals; you can use a boolean flag to
> distinguish between an inertial pan gesture and a regular one.

As I said, I'd rather have a signal that immediately marks the end of the proper pan events sequence, both to not lose frames and to minimize the state-tracking for users, but if you prefer I would be fine without it.

Thanks for the quick response!

Created attachment 221663 [details] [review]
Get rid of GTimeVal and use g_get_monotonic_time()

Created attachment 222399 [details] [review]
gesture-action: add allow-none annotations to getters

Created attachment 222400 [details] [review]
gesture-action: add _get_motion_delta()/_get_velocity()

Add some accessors to simplify common tasks for GestureAction users:

• clutter_gesture_action_get_motion_delta() to get the delta
  on the X and Y axis in stage coordinates since the last motion
  event, and the scalar distance travelled;

• clutter_gesture_action_get_velocity() to get an estimate of the
  speed of the last motion event along the X and Y axis and as a
  scalar value in pixels per millisecond.

Created attachment 222401 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

PanAction is a GestureAction-subclass that implements the panning
concept for scrollable actors, with the ability to emit interpolated
signals to emulate the kinetic inertia of the panning. PanAction provides:

• pan signal, notifying users of the panning gesture status;

• pan-stopped signal, emitted at the end of the interpolated phase
  of the panning gesture, if enabled;

• pan-axis property, to allow constraining the dragging to a specific
  axis;

• interpolated property, to enable or disable the inertial behaviour;

• deceleration property, to customize the rate at which the momentum
  of the panning will be slowed down;

• acceleration-factor property, applied to the inertial momentum when
  starting the interpolated sequence.

An interactive test is also provided.

I've reworked the PanAction patches:
• handle motion delta and velocity in GestureAction (naive implementation, not using a motion buffer);
• reduce the signals to just ::pan and ::pan-stopped (the latter only if interpolating);
• estimate duration and target position of the inertial movement and then just use the progress of the timeline for the motion coordinates;
• add clutter_pan_action_real_pan() to move the actor content on ::pan.

For the latter the following code doesn't seem to work as I thought, what should I do to move the actor contents?

clutter/clutter-pan-action.c:298 
  clutter_actor_get_child_transform (actor, &transform);
  cogl_matrix_translate (&transform, dx, dy, 0.0f);
  clutter_actor_set_child_transform (actor, &transform);

Created attachment 222405 [details] [review]
Oops, I left two references to ::pan-end around, fixed.

Created attachment 222528 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

Fixed the return value of the ::pan handler in the example program.

For future reference, the _set_child_transform() issue has been solved by http://git.gnome.org/browse/clutter/commit/?id=3937a7

Created attachment 222554 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

Added a missing documentation document and wired up gtk-doc.

Created attachment 222555 [details] [review]
gesture-action: add _get_motion_delta()/_get_velocity()

Add symbols to export list and gtk-doc.

Review of attachment 222399 [details] [review]:

AFAIR, this should not be needed except maybe for Vala - but I guess it's perfectly fine.

Review of attachment 222554 [details] [review]:

looks good, thanks! some minor style issues left.

::: clutter/clutter-pan-action.c
@@ +73,3 @@
+  PAN_STATE_PANNING,
+  PAN_STATE_INTERPOLATING
+} _ClutterPanState;

there's no need to use a leading underscore; you can use 'PanState' for the name.

@@ +81,3 @@
+  _ClutterPanState state;
+
+  gboolean should_interpolate;

this should be a:

  guint should_interpolate : 1;

and placed at the end of the struct.

@@ +85,3 @@
+  /* Variables for storing acceleration information */
+  ClutterTimeline     *deceleration_timeline;
+  gfloat               target_x;

please, don't leave this whitespace between type and name.

@@ +153,3 @@
+  ClutterActor *actor;
+
+  g_object_unref (timeline);

you can use g_clear_object() instead of g_object_unref() + set to NULL.

@@ +162,3 @@
+static void
+on_deceleration_new_frame (ClutterTimeline     *timeline,
+                           gint                 frame_num,

"frame_num" should be "elapsed_time"

@@ +232,3 @@
+  guint duration;
+  const gfloat min_velocity = 0.1f; // measured in px/ms
+  const gfloat reference_fps = 60.0f; // the fps assumed for the deceleration rate

you probably want to have these two as static consts declared at the top of the file; we may even allow configuration through properties later on.

@@ +251,3 @@
+   * with frame_per_second = 60 and decay_per_frame = 0.95, tau ~= 325ms
+   * see http://ariya.ofilabs.com/2011/10/flick-list-with-its-momentum-scrolling-and-deceleration.html */
+  tau = 1000.0f / (reference_fps * -logf(priv->deceleration_rate));

missing space between logf and parenthesis.

@@ +259,3 @@
+
+  /* Target point: x(t) = v(0) * tau * [1 - exp(-t/tau)] */
+  priv->target_x = velocity_x * priv->acceleration_factor * tau * (1 - exp(-(float)duration/tau));

missing spaces between exp and parenthesis; missing space between duration, /, and tau. the cast to float should be done before the '-'.

@@ +260,3 @@
+  /* Target point: x(t) = v(0) * tau * [1 - exp(-t/tau)] */
+  priv->target_x = velocity_x * priv->acceleration_factor * tau * (1 - exp(-(float)duration/tau));
+  priv->target_y = velocity_y * priv->acceleration_factor * tau * (1 - exp(-(float)duration/tau));

same as above.

@@ +330,3 @@
+    case PROP_INTERPOLATE :
+      clutter_pan_action_set_interpolate (self,
+          g_value_get_boolean (value));

do not break the line here.

@@ +335,3 @@
+    case PROP_DECELERATION :
+      clutter_pan_action_set_deceleration (self,
+          g_value_get_double (value));

do not break the line here.

@@ +340,3 @@
+    case PROP_ACCELERATION_FACTOR :
+      clutter_pan_action_set_acceleration_factor (self,
+          g_value_get_double (value));

do not break the line here.

@@ +388,3 @@
+    {
+      clutter_timeline_stop (priv->deceleration_timeline);
+      g_object_unref (priv->deceleration_timeline);

you should use g_clear_object(). disposing the timeline will also stop it, so there's no need to manually stop it. if you don't want to emit signals, disconnect the signal handlers before disposing the timeline.

@@ +410,3 @@
+        {
+          clutter_timeline_stop (priv->deceleration_timeline);
+          g_object_unref (priv->deceleration_timeline);

same as above - use g_clear_object() instead.

@@ +520,3 @@
+   * interpolation phase of the pan, after the drag has ended and
+   * the :interpolate property was set to %TRUE.
+   *

missing Return value: annotation.

@@ +561,3 @@
+    G_TYPE_INSTANCE_GET_PRIVATE (self, CLUTTER_TYPE_PAN_ACTION,
+                                 ClutterPanActionPrivate);
+  priv->deceleration_rate = 0.95f;

the deceleration rate default value does not match the property's GParamSpec.

@@ +649,3 @@
+  priv = self->priv;
+
+  if (priv->should_interpolate == should_interpolate)

if you're comparing directly, you should do:

  should_interpolate = !!should_interpolate;

before the if().

@@ +701,3 @@
+  priv = self->priv;
+
+  if (rate == priv->deceleration_rate)

comparing double precision values directly with a '==' won't really work, especially if we're talking about small deltas.

you can either use abs(rate - priv->deceleration_rate) < float_epsilon, or use memcmp().

or you could just not bother.

@@ +737,3 @@
+void
+clutter_pan_action_set_acceleration_factor (ClutterPanAction *self,
+                                                      gdouble                    factor)

extraneous whitespace.

@@ +746,3 @@
+  priv = self->priv;
+
+  if (factor == priv->acceleration_factor)

same as above: direct comparison of doubles won't really work.

::: examples/pan-action.c
@@ +15,3 @@
+  clutter_actor_set_size (content, 720, 720);
+
+  pixbuf = gdk_pixbuf_new_from_file (TESTS_DATADIR G_DIR_SEPARATOR_S "redhand.png", NULL);

we should probably get a better image than this.

@@ +17,3 @@
+  pixbuf = gdk_pixbuf_new_from_file (TESTS_DATADIR G_DIR_SEPARATOR_S "redhand.png", NULL);
+  image = clutter_image_new ();
+  clutter_image_set_data (CLUTTER_IMAGE (image),

the indentation is wrong. you have to line up all arguments with the opening parenthesis.

@@ +20,3 @@
+      gdk_pixbuf_get_pixels (pixbuf),
+      gdk_pixbuf_get_has_alpha (pixbuf)
+      ? COGL_PIXEL_FORMAT_RGBA_8888

you should indent the ternary operator arguments by one level.

@@ +29,3 @@
+
+  clutter_actor_set_content_scaling_filters (content,
+      CLUTTER_SCALING_FILTER_TRILINEAR,

wrong indentation.

@@ +109,3 @@
+  /* create a new stage */
+  stage = clutter_stage_new ();
+  clutter_stage_set_title (CLUTTER_STAGE (stage), "Scroll Actor");

this should probably be titled "Pan Action"

Review of attachment 222555 [details] [review]:

looks good.

Created attachment 222558 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

Re-spin. I ditched the float checks and now I just trigger property
notifications on every setter call.

Review of attachment 222558 [details] [review]:

looks great, thanks!

even if we're in API freeze, I really want to have this before freezing the 1.x branch, so let's get it in.

::: examples/pan-action.c
@@ +3,3 @@
+#include <clutter/clutter.h>
+
+ClutterPoint initial_scroll = CLUTTER_POINT_INIT (0, 120);

this is not used anywhere in the example.

Comment on attachment 222399 [details] [review]
gesture-action: add allow-none annotations to getters

e8e91b62c896223d8f7bc4421862b87ef002a972

Comment on attachment 222555 [details] [review]
gesture-action: add _get_motion_delta()/_get_velocity()

436ebb2716b24743b3873aa7b330073c48793b86

Comment on attachment 222558 [details] [review]
pan-action: add PanAction, to handle panning in scrollable actors

9ca06d2895154eb2c985b85df186db3ade1a5e1e