GNOME Bugzilla – Bug 689061
Some fixes and API additions for ClutterPanAction
Last modified: 2012-11-30 23:03:18 UTC
I ran into these when trying to port gnome-shell to stop using custom swipe scrolling code.
Created attachment 229873 [details] [review] pan-action, zoom-action: Use the right accumulator The documentation said that you should return TRUE to mark that the action was handled, but the code did the reverse.
Created attachment 229874 [details] [review] pan-action: Add a simple convenience function to retrieve delta/coords This is a simple helper designed to ease the implementation of alternate implementations of the "pan" signal handler.
Review of attachment 229873 [details] [review]: ::: clutter/clutter-pan-action.c @@ -523,3 @@ * the :interpolate property was set to %TRUE. * - * Return value: TRUE if the pan action has been handled by one of The continue accumulator is fine as it is also used by GestureAction, RotateAction and ZoomAction, we just need to change the documentation. Would you fix the ClutterZoomAction::zoom documentation too?
Review of attachment 229874 [details] [review]: I'm a bit worried about the naming override - but I guess it works well enough, given the intent. you also need to add the new symbols to clutter.symbols and the clutter-sections.txt for the API reference.
Created attachment 229943 [details] [review] pan-action, zoom-action: Fix documentation for signals The documentation said that you should return TRUE to mark that the action was handled, but the code did the reverse. Change the documentation to reflect what all the other gestures do.
Created attachment 229944 [details] [review] pan-action: Add a simple convenience function to retrieve delta/coords This is a simple helper designed to ease the implementation of alternate implementations of the "pan" signal handler.
Review of attachment 229943 [details] [review]: Looks good to me. Maybe remove the extra space at the beginning of the second line in both comments before pushing if ebassi agrees?
Review of attachment 229943 [details] [review]: looks good to me; the space is fine - we usually indent the Return value: annotation.
Review of attachment 229944 [details] [review]: ::: clutter/clutter-pan-action.c @@ +829,3 @@ + * clutter_pan_action_get_motion_delta: + * @self: A #ClutterPanAction + * @device: currently unused, set to 0 s/device/point and not really unused, it's the point index. TBH I wouldn't expose that and internally assume 0 by now. This could prove useful in case we want to make ClutterPanAction respond on two fingers gestures by calling clutter_gesture_action_set_n_touch_points() and return the delta of the averaged coords. @@ +879,3 @@ + * clutter_pan_action_get_motion_coords: + * @self: A #ClutterPanAction + * @device: currently unused, set to 0 The same applies here. ::: clutter/clutter.symbols @@ +1062,3 @@ clutter_pan_action_get_deceleration +clutter_pan_action_get_motion_delta +clutter_pan_action_get_motion_coords This should be alphabetically ordered, or at least try to be. :)
I copied the documentation and whatever else from ClutterGestureAction. I didn't add it in the first patch I don't think, but ebassi said that if it has the same method name, it should have the same signature. Hence why I calculate distance and all that, too. I figured that when people did the point renaming, it'd be renamed then, otherwise we'll have an intermediate spot where "point" itself as "device", which looks wrong, even if it's technically correct.
(In reply to comment #10) > Review of attachment 229944 [details] [review]: > > ::: clutter/clutter-pan-action.c > @@ +829,3 @@ > + * clutter_pan_action_get_motion_delta: > + * @self: A #ClutterPanAction > + * @device: currently unused, set to 0 > > s/device/point and not really unused, it's the point index. > TBH I wouldn't expose that and internally assume 0 by now. This could prove > useful in case we want to make ClutterPanAction respond on two fingers gestures > by calling clutter_gesture_action_set_n_touch_points() and return the delta of > the averaged coords. the whole point of the exercise is to have the exact same signature (i.e. an override) as ClutterGestureAction.get_motion_delta() without having to make *that* a virtual function. we have three option, here: • make get_motion_delta() and get_motion_coords() two virtual functions in Clutter.GestureAction, have a default implementation in the base class, and an overridden one in Clutter.PanAction that takes into account the action's state; • make get_motion_delta() and get_motion_coords() in Clutter.PanAction behave exactly like Clutter.GestureAction, including their signature and return value, but document that the @point argument is currently ignored, like we used to do for the GestureAction methods before we had multi-touch point support; • add two new functions in Clutter.PanAction, with different names; there's also the fourth (secret) option of having gesture states [ "idle", "tracking", "interpolating" ] as a (private) property on GestureAction, and let the sub-classes set the state, so that Clutter.GestureAction.get_motion_* can check the state and do the computation in the base class without requiring a virtual function. this is getting far too complicated for a convenience function.
Comment on attachment 229943 [details] [review] pan-action, zoom-action: Fix documentation for signals Attachment 229943 [details] pushed as 90a2401 - pan-action, zoom-action: Fix documentation for signals
Created attachment 230241 [details] [review] pan-action: Add a simple convenience function to retrieve delta/coords This is a simple helper designed to ease the implementation of alternate implementations of the "pan" signal handler.
Review of attachment 230241 [details] [review]: looks good
Attachment 230241 [details] pushed as 75b521d - pan-action: Add a simple convenience function to retrieve delta/coords