GNOME Bugzilla – Bug 707982
Add PanAxis mode that automatically pins scroll based on initial movement
Last modified: 2015-06-11 18:51:20 UTC
This code is inspired by the implementation of the same feature for the Mx toolkit's MxKineticScrollView. See commit 4d08771.
Created attachment 254792 [details] [review] Add PanAxis mode that automatically pins scroll based on initial movement
Review of attachment 254792 [details] [review]: I think it's a bit too late to have this for 1.16, but we can land it in 1.x after the 3.10 release. ::: clutter/clutter-enums.h @@ +875,3 @@ * @CLUTTER_PAN_X_AXIS: Set a constraint on the X axis * @CLUTTER_PAN_Y_AXIS: Set a constraint on the Y axis + * @CLUTTER_PAN_AUTO_AXIS: Constrain paning automatically based on initial movement the documentation and the enumeration value do not match. ::: clutter/clutter-pan-action.c @@ +108,3 @@ guint should_interpolate : 1; + + guint should_pin_tested : 1; "should_pin_tested" is not very descriptive. maybe "should_check_pin". @@ +155,3 @@ + gfloat delta_x; + gfloat delta_y; + priv->pin_state = SCROLL_PINNED_NONE; we do prefer using G_PI, as not every M_PI macro is defined on non-Linux platforms. @@ +165,3 @@ + drag_angle = M_PI_2; + + gfloat scroll_threshold = M_PI_4/2; coding style: space after 'if', and no space between parenthesis. @@ +817,2 @@ /** + * clutter_pan_action_get_constrained_motion_delta: do we really need this as a public function? @@ +825,3 @@ + * Retrieves the delta, in stage space, dependent on the current state + * of the #ClutterPanAction, and respecting the constraint specified by the + * @delta_x: (out) (allow-none): return location for the X delta in order for the link to work, you need to use #ClutterPanAction:pan-axis. @@ +828,3 @@ + * + * Return value: the distance since last motion event + * @delta_y: (out) (allow-none): return location for the Y delta missing Since: annotation. @@ +852,3 @@ + case CLUTTER_PAN_AXIS_AUTO: + if (priv->pin_state == SCROLL_PINNED_VERTICAL) + gfloat *delta_x, coding style: only two spaces for indentation. @@ +854,3 @@ + *delta_x = 0.0f; + else if (priv->pin_state == SCROLL_PINNED_HORIZONTAL) + gfloat *delta_y) coding style: only two spaces for indentation. ::: clutter/clutter-pan-action.h @@ +121,3 @@ + gfloat *delta_x, + gfloat *delta_y); + gdouble factor); missing CLUTTER_AVAILABLE_IN_1_16 annotation.
Review of attachment 254792 [details] [review]: Thanks for the review, I'll have a patch dealing with the comments up in a bit! ::: clutter/clutter-pan-action.c @@ +817,2 @@ /** + * clutter_pan_action_get_constrained_motion_delta: WebKit Clutter is using the pan action and others for implementing its gestures support, but it doesn't use them for implementing the actual translation - that is dealt with inside WebKit. One of the reasons is it might be panning an in-page scrollable instead of the main frame, for instance, but there are other reasons to let WebCore do the gesture handling after it's been recognized. What we did before was just querying the motion delta and sending it down to WebCore's event handler. To take advantage of the axis pinning we either need to know for what axis the delta should be disregarded, or get the already-constrained motion delta. I think just getting the processed motion delta works well and makes the code simpler, but we could deal with it by exposing which axis should be disregarded instead, what do you think?
Created attachment 255054 [details] [review] Add PanAxis mode that automatically pins scroll based on initial movement This code is inspired by the implementation of the same feature for the Mx toolkit's MxKineticScrollView. See commit 4d08771.
Created attachment 273237 [details] [review] Add PanAxis mode that automatically pins scroll based on initial movement I've rebased the patch on top of the current clutter-1.18 branch (as I'm not sure about the status of master) and got rid of the `should_check_pin` member by adding one more state to PinState.
Ping, is there something we need to improve on this to get it landed?
Review of attachment 273237 [details] [review]: ::: clutter/clutter-enums.h @@ +1008,3 @@ * @CLUTTER_PAN_X_AXIS: Set a constraint on the X axis * @CLUTTER_PAN_Y_AXIS: Set a constraint on the Y axis + * @CLUTTER_PAN_AXIS_AUTO: Constrain panning automatically based on initial movement You should add 'Available since: 1.24' in the enumeration description. ::: clutter/clutter-pan-action.c @@ +162,3 @@ + + if (delta_x != 0.0f) + drag_angle = atanf(delta_y / delta_x); coding style: needs a space after atanf. @@ +869,3 @@ + * Return value: the distance since last motion event + * + * Since: 1.18 Since: 1.24 @@ +892,3 @@ + + case CLUTTER_PAN_AXIS_AUTO: + if (priv->pin_state == SCROLL_PINNED_VERTICAL) You need to check if delta_x and delta_y are non-NULL. The easiest way to handle this is to use internal delta_x, delta_y variables, and then copy them to the passed in out arguments. ::: clutter/clutter-pan-action.h @@ +143,3 @@ gfloat *motion_x, gfloat *motion_y); +CLUTTER_AVAILABLE_IN_1_18 CLUTTER_AVAILABLE_IN_1_24 @@ +144,3 @@ gfloat *motion_y); +CLUTTER_AVAILABLE_IN_1_18 +gfloat clutter_pan_action_get_constrained_motion_delta (ClutterPanAction *self, Is it really necessary to have it as a public function?
Review of attachment 273237 [details] [review]: Sorry for the delay, will upload an updated patch right now =) ::: clutter/clutter-pan-action.c @@ +892,3 @@ + + case CLUTTER_PAN_AXIS_AUTO: + if (priv->pin_state == SCROLL_PINNED_VERTICAL) get_motion_delta below doesn't do that FWIW, but I can see cases in which you'd only want to pass one out variable indeed. ::: clutter/clutter-pan-action.h @@ +144,3 @@ gfloat *motion_y); +CLUTTER_AVAILABLE_IN_1_18 +gfloat clutter_pan_action_get_constrained_motion_delta (ClutterPanAction *self, It is unless we expose another way of calculating this. As I explained above we rely on the PanAction for getting the events but how to handle them is up to WebCore - we do not rely on it for doing the actual scrolling translation, since the scrolling might actually be an in-page element, not the WebView actor's contents, for instance..
(In reply to Gustavo Noronha (kov) from comment #8) > + case CLUTTER_PAN_AXIS_AUTO: > + if (priv->pin_state == SCROLL_PINNED_VERTICAL) > > get_motion_delta below doesn't do that FWIW, but I can see cases in which > you'd only want to pass one out variable indeed. erm, of course it does!
Created attachment 305074 [details] [review] Add PanAxis mode that automatically pins scroll based on initial movement
Review of attachment 305074 [details] [review]: Needs a couple of fixes, but it looks good. ::: clutter/clutter-pan-action.c @@ +860,3 @@ + * @point: the touch point index, with 0 being the first touch + * point received by the action + * @delta_x: (out) (allow-none): return location for the X delta The (allow-none) annotation is deprecated; this should be (optional). @@ +861,3 @@ + * point received by the action + * @delta_x: (out) (allow-none): return location for the X delta + * @delta_y: (out) (allow-none): return location for the Y delta Same as above. @@ +878,3 @@ +{ + ClutterPanActionPrivate *priv; + * You should initialize delta_x and delta_y to 0.f.
Created attachment 305093 [details] [review] Add PanAxis mode that automatically pins scroll based on initial movement Thanks! Here you go =)
Review of attachment 305093 [details] [review]: Looks good.
Thanks! I noticed I had forgotten to add the new method to the docs, so I took the liberty of doing that. Landed as 0c75e178145c3296f05deefed34be3691b1ffb3b