Created attachment 220295 [details] [review]
ClutterDragAction: allow constraining the movement of the dragged actor

Review of attachment 220295 [details] [review]:

Considering that you can only drag one axis at a time, maybe this should be separate min/max properties.

(In reply to comment #2)
> Review of attachment 220295 [details] [review]:
> 
> Considering that you can only drag one axis at a time, maybe this should be
> separate min/max properties.

This is true for how ClutterDragAction is used in the shell, but not in general. In fact, if drag-axis is not set, the actor can be dragged in any direction.

Huh. I didn't realize that.

Review of attachment 220295 [details] [review]:

the original design of the DragAction had the concept of a "drag-area" similar to the drag-axis constraint; sadly, it escalated fast because you need both a screen-relative area and a parent-relative one, so it had to be dumped before DragAction had been pushed to master.

::: clutter/clutter-actor.c
@@ +9840,3 @@
+
+/*
+ * _clutter_actor_move_by_within:

*yuck*.

why do you need a separate function at all? this code is not shared used anywhere else, and it can be used directly inside the ClutterDragAction; you can even access the LayoutInfo directly from there, if you don't want to go through clutter_actor_get_position().

::: clutter/clutter-drag-action.c
@@ +126,3 @@
   PROP_DRAG_HANDLE,
   PROP_DRAG_AXIS,
+  PROP_DRAG_BOX,

if the DragBox can be unset, you need a separate read-only property, DRAG_BOX_SET. see how we handle ClutterActor:background-color.

@@ +1210,3 @@
+ */
+ClutterActorBox *
+clutter_drag_action_get_drag_box (ClutterDragAction *action)

this is not consistent with how Clutter handles boxed type getters.

the proper way would be:

gboolean
clutter_drag_action_get_drag_box (ClutterDragAction *a, ClutterActorBox *b);

though, ClutterRect is the correct type that in the future will replace all usage of ClutterActorBox.

@@ +1229,3 @@
+void
+clutter_drag_action_set_drag_box (ClutterDragAction *action,
+				  ClutterActorBox   *box)

the box argument ought to be const.

@@ +1233,3 @@
+  g_return_if_fail (CLUTTER_IS_DRAG_ACTION (action));
+
+  if (action->priv->drag_box)

don't copy boxed types around using the allocator.

::: clutter/clutter-drag-action.h
@@ +130,3 @@
                                                         gfloat            *motion_y);
 
+ClutterActorBox *clutter_drag_action_get_drag_box      (ClutterDragAction *action);

missing CLUTTER_AVAILABLE_IN_1_12 annotations.

@@ +131,3 @@
 
+ClutterActorBox *clutter_drag_action_get_drag_box      (ClutterDragAction *action);
+void             clutter_drag_action_set_drag_box      (ClutterDragAction *action,

missing CLUTTER_AVAILABLE_IN_1_12 annotations.

@@ +132,3 @@
+ClutterActorBox *clutter_drag_action_get_drag_box      (ClutterDragAction *action);
+void             clutter_drag_action_set_drag_box      (ClutterDragAction *action,
+							ClutterActorBox   *box);

do not use ClutterActorBox for newly added API; use ClutterRect instead.

by the way, if you need this sooner rather than later, you can use the ::drag-progress signal to constrain the dragging - just check if the delta would take you outside of the allotted area, and return FALSE to prevent the ::drag-motion signal being emitted.

Created attachment 221065 [details] [review]
ClutterDragAction: allow constraining the movement of the dragged actor

Allow setting a ClutterRect on the drag action and force the
dragged actor's position to be always within that rectangle (relative
to the actor's parent).

Review of attachment 221065 [details] [review]:

looks okay; it would be nice to have an example, but I guess we can add it later.

feel free to commit after fixing the remarks.

::: clutter/clutter-drag-action.c
@@ +1250,3 @@
+  g_return_val_if_fail (CLUTTER_IS_DRAG_ACTION (action), FALSE);
+
+  *drag_area = action->priv->drag_area;

you need "if (drag_area != NULL)" before the assignment, otherwise you'll get a segfault if somebody passes a NULL pointer to drag_area.

@@ +1274,3 @@
+  priv = action->priv;
+
+  if (drag_area)

explicit comparison against NULL, please.

@@ +1281,3 @@
+  else
+    {
+      priv->drag_area_set = FALSE;

you can drop the curly braces here.

Attachment 221065 [details] pushed as f99d48a - ClutterDragAction: allow constraining the movement of the dragged actor