GNOME Bugzilla – Bug 678427
Zoom action
Last modified: 2012-08-20 17:30:59 UTC
Created attachment 216766 [details] [review] patch v1 While playing with the multi touch on Android, I thought it would be cool to have a "zoom" action, pretty much like what every smart phone have. Here is a try that implements a drag as long as you have a single touch point and a zoom/scale once 2 touch points are on the actor. I'm looking for reviews about the API, mostly the zoom_begin/zoom_update/zoom_end signals. I'm not sure emitting the distance between points is enough, maybe replacing that with the couple of points. But what structure?
*** Bug 678426 has been marked as a duplicate of this bug. ***
Also it would be easy to make this word easily with 2 pointers as well as 2 touch points (saving that for later).
Review of attachment 216766 [details] [review]: thanks for looking at this. why are you not using ClutterGestureAction as the base class? the -begin, -progress, -end signals are there already. plus, *six* signals? and a duplicate of the DragAction? dragging should be performed by a separate DragAction - that's why we allow multiple actions to work together. we do need a way to structure themselves in a chain, so that one is checked first - and we can use the ClutterActorMeta priority that we already use for ClutterEffect for that. the way the GestureAction API was envisioned, while the touch support was in progress in x11, was to have that class handle the number of required touch points, as well as emitting gesture-begin, gesture-progress, and gesture-end; a subclass of ClutterGestureAction would set the minimal amount of touch points required for it to work inside its own constructor, e.g.: clutter_gesture_action_set_n_touch_points (zoom_action, 2); which then would make GestureAction switch to using touch-event, and keep track of the touch-event:begin events it received before emitting gesture-begin, and emitting gesture-cancel in case it received a touch-event:end before another touch-event:begin or a touch-event:update. what remains to be defined is whether to store the ClutterEvent emitted before gesture-cancel is emitted, and where to store them - either as a facility of ClutterAction itself, or as part of ClutterGestureAction.
Fair point about reusing ClutterGestureAction. The only problem I have here is adding more than one action to an actor. Because of the DragAction/GestureAction both using captured-event signals on the stage, I fail to see how to make things work at the moment.
Actions should not stop event propagation, so every action should get all the events; the DragAction should only work if there is a single touch point, and the ZoomAction should only work with two or more touch points, so we have a way to control whether one or the other will actually handle the actor. you're right, though: we need API to define the chain and allow a ZoomAction to take precedence over a DragAction. right now, every ClutterActorMeta has a priority which is used by the ClutterMetaGroup that stores them, but all ClutterActions are just registering event handlers on the actor to which they are attached to in the order of attachment; this means that we cannot use the ClutterMetaGroup order to control the order of the signal connection. one thing I wanted to do was to move the ClutterAction registration to the stage to which the actor belongs on map, and then let the Stage deliver the events to the ClutterAction themselves, instead of using the actor event-related signals; you'd still attach the Action to the Actor, but on ::map the actor would add the actions it holds to a list on the stage. during the event processing, the Stage would go through the list of actions for the actor that is going to be the source of the Event, and call an ::process-event virtual function on each ClutterAction. the ClutterActions would be sorted using the priority, so that the user could set the ZoomAction to have a higher priority than the DragAction. this is similar to how constraints are processed during the allocation of an actor.
The main reason I added dragging into the ZoomAction is to be able to continue to drag once you've released one of the finger. I don't see how I can make that work by using 2 actions. Any thought?
Following the logic in comment 3, shouldn't DragAction be rewritten as a subclass of GestureAction?
(In reply to comment #7) > Following the logic in comment 3, shouldn't DragAction be rewritten as a > subclass of GestureAction? we cannot do that in an ABI compatible way for 1.x; and even then, I'm not entirely sure that 'dragging' is a gesture, though it does overlap with padding; iOS uses the term 'panning' for both panning and dragging, so I guess we could introduce a new PanAction in 1.x. (In reply to comment #6) > The main reason I added dragging into the ZoomAction is to be able to continue > to drag once you've released one of the finger. I don't see how I can make that > work by using 2 actions. Any thought? that would be part of the fallback chain that I outlined in comment 5: if the user releases a touch point, and the number of touch points drops under the required value, the zoom ends, and control gets passed to the next action. I think the fallback chain API should be modelled over the equivalent API inside the UIGestureRecognizer class in iOS: http://developer.apple.com/library/ios/#documentation/EventHandling/Conceptual/EventHandlingiPhoneOS/GestureRecognizers/GestureRecognizers.html but while UIGestureRecognizers are registered on the UIView explicitly in iOS, we'd register them implicitly when an actor gets mapped, i.e. inside clutter_actor_real_map: stage = self.get_stage() for action in self.actions: stage.register_action(action, self) and inside clutter_actor_real_unmap: stage = self.get_stage() for action in self.actions: stage.unregister_action(action, self) as well as calling register_action() from Clutter.Actor.add_action(), and unregister_action() from the Clutter.Actor.remove_action() method, if the actor is mapped.
Created attachment 217331 [details] [review] patch v2 Rewritten on top of GestureAction and removed the code dealing with the dragging.
This ZoomAction doesn't seem to work properly if it's applied to an actor with a that gets an initial rotation for one of its parent. I guess that can be fixed later, it does not involve changing the API/ABI.
Any news on this?
I'm hoping finishing this before the end of Guadec, but it's already functional code. I need to rework the code in the same way than I did on the rotation action, to allow someone to override the behavior of the zoom action. There is also a problem of if you're using this action within a non-centric rotated actor, but that isn't trivial to fix and a very unlikely case...
Any update on this ?
Created attachment 220948 [details] [review] patch v3 Yes, finally found some time to clean that up.
Review of attachment 220948 [details] [review]: thanks! looks okay to me, except for a couple of issue and the documentation. you also need to update clutter.symbols, and eventually doc/reference/clutter/clutter-sections.txt and doc/reference/clutter/clutter.types. ::: clutter/clutter-marshal.list @@ +18,3 @@ VOID:INT64,INT64,FLOAT,BOOLEAN VOID:OBJECT +VOID:OBJECT,BOXED,DOUBLE wrong marshaller - should be BOOLEAN:OBJECT,BOXED,DOUBLE ::: clutter/clutter-zoom-action.c @@ +31,3 @@ + * + * #ClutterZoomAction is a sub-class of #ClutterAction that implements + * all the necessary logic for dragging & zooming actors. the description has to be rewritten. @@ +292,3 @@ + &in, &out); + + clutter_actor_move_by (actor, instead of using move_by() can you use the set_translation() transformation? this would at least avoid a relayout. @@ +351,3 @@ + + g_type_class_add_private (klass, sizeof (ClutterZoomActionPrivate)); + too much whitespace. @@ +383,3 @@ + + /** + * ClutterZoomAction::zoom-update: should be ::zoom: @@ +399,3 @@ + * The default handler of the signal will call + * clutter_actor_set_scale() either on @actor or, if set, of + * #ClutterZoomAction:drag-handle using the ratio of the first there is no drag-handle property, so this paragraph should just go away. @@ +404,3 @@ + * this signal and call g_signal_stop_emission_by_name() from within + * your callback. + * missing Return value annotation. @@ +412,3 @@ + G_SIGNAL_RUN_LAST, + G_STRUCT_OFFSET (ClutterZoomActionClass, zoom), + _clutter_boolean_handled_accumulator, NULL, this should be _clutter_boolean_continue_accumulator. @@ +413,3 @@ + G_STRUCT_OFFSET (ClutterZoomActionClass, zoom), + _clutter_boolean_handled_accumulator, NULL, + _clutter_marshal_VOID__OBJECT_BOXED_DOUBLE, the marshaller is wrong - it should have a BOOLEAN return value. @@ +460,3 @@ + g_return_if_fail (axis >= CLUTTER_DRAG_AXIS_NONE && + axis <= CLUTTER_DRAG_Y_AXIS); + missing: if (action->priv->axis == axis) return; @@ +461,3 @@ + axis <= CLUTTER_DRAG_Y_AXIS); + + action->priv->zoom_axis = axis; missing g_object_notify_by_pspec(). ::: clutter/clutter-zoom-action.h @@ +31,3 @@ + +#include <clutter/clutter-event.h> +#include <clutter/clutter-drag-action.h> if you need the ClutterEvent and ClutterDragAxis types, you can just include <clutter/clutter-types.h>. @@ +70,3 @@ + * @zoom_begin: class handler of the #ClutterZoomAction::zoom-begin signal + * @zoom_update: class handler of the #ClutterZoomAction::zoom-update signal + * @zoom_end: class handler of the #ClutterZoomAction::zoom-end signal need to remove all of these from the gtk-doc annotation - and add a @zoom one. @@ +96,3 @@ +}; + +GType clutter_zoom_action_get_type (void) G_GNUC_CONST; all the public symbols should have a CLUTTER_AVAILABLE_IN_1_12 annotation.
another thing I realized when looking at the patch while trying to amend it: ZoomAction should not reuse ClutterDragAxis. a new enumeration should be introduced for its use - e.g. ClutterZoomAxis.
Created attachment 221838 [details] [review] patch v4 Hopefully I didn't forget anything.
Review of attachment 221838 [details] [review]: looks okay to me - the last couple of comments can be fixed before pushing it to master. ::: clutter/clutter-enums.h @@ +1334,3 @@ + */ +typedef enum { /*< prefix=CLUTTER_ZOOM >*/ + CLUTTER_ZOOM_AXIS_NONE = 0, I'd call this CLUTTER_ZOOM_BOTH, like ClutterAlignAxis. ::: clutter/clutter-zoom-action.c @@ +168,3 @@ + priv->zoom_initial_distance = sqrt (dx * dx + dy * dy); + + clutter_actor_get_position (actor, this should be get_translation(). @@ +220,3 @@ + ClutterZoomActionPrivate *priv = ((ClutterZoomAction *) action)->priv; + + clutter_actor_set_position (actor, priv->initial_x, priv->initial_y); this should be set_translation() with the initial transformation.
attachment 221838 [details] [review] pushed to master with minimal fixes.