Created attachment 366039 [details] [review]
Add PhotosToolCropHelper

Created attachment 366040 [details] [review]
tool-crop: Animate changes to orientation of crop rectangle

Review of attachment 366040 [details] [review]:

This looks really nice!

One overall comment: I think that we should differentiate between the co-ordinates that are actually used to draw the crop rectangle (eg., in photos_tool_crop_surface_draw), and the ones that are passed to gegl:crop (through g_action_activate).

The former is meant for drawing the intermediate stages of the animation. However, if g_action_activate is invoked in the middle of an ongoing animation (it's unlikely, but still), then we shouldn't be using the intermediate rectangles to crop the image. The image should always be cropped with the "final" co-ordinates.

You'll see a similar distinction in ImageView. See photos_image_view_set_best_fit and photos_image_view_set_zoom. The best-fit and zoom values exposed through ImageView's public API are always the "final" values. The intermediate values used to drive the animation (ie. the *_visible values) are kept private and only used for drawing.

::: src/photos-tool-crop.c
@@ +861,3 @@
+  /* We might decide to animate every crop preset change in future.
+   * But for now, stick to only orientation changes.
+   */

That's a good idea! We can do it in this patch itself. :)

Created attachment 366343 [details] [review]
tool-crop: Separate callbacks for orientation toggle buttons

Change in orientation fires the same callback twice, which starts and
stops the animation immediately. Separating the callbacks helps to
simplify that problem in following commits.

Created attachment 366344 [details] [review]
tool-crop: Animate crop rectangle when preset is changed

Created attachment 366369 [details] [review]
tool-crop: Animate crop rectangle when preset is changed

Fixed a small visual glitch

Review of attachment 366039 [details] [review]:

::: src/photos-tool-crop-helper.c
@@ +101,3 @@
+                                                        "aspect_ratio",
+                                                        "Crop aspect ratio",
+                                                        -G_MAXDOUBLE,

Nitpick: aspect ratio values can never be negative. Zero is probably a better minimum, even though it's unlikely to ever actually be zero.

(In reply to Debarshi Ray from comment #7)
> Review of attachment 366039 [details] [review] [review]:
> 
> ::: src/photos-tool-crop-helper.c
> @@ +101,3 @@
> +                                                        "aspect_ratio",
> +                                                        "Crop aspect ratio",
> +                                                        -G_MAXDOUBLE,
> 
> Nitpick: aspect ratio values can never be negative. Zero is probably a
> better minimum, even though it's unlikely to ever actually be zero.

Oops! I forgot that the absence of an aspect ratio constraint is represented by -1.0.

Review of attachment 366369 [details] [review]:

::: src/photos-tool-crop.c
@@ +328,3 @@
 
+  self->crop_height = sqrt (old_area / self->crop_aspect_ratio_visible);
+  self->crop_width = sqrt (old_area * self->crop_aspect_ratio_visible);

It's not enough to only have a *_visible counterpart of self->crop_aspect_ratio. We need to do the same for the size and position of the crop rectangle.

What we really want is to differentiate between the co-ordinates that are actually used to draw the crop rectangle (eg., in photos_tool_crop_surface_draw), and the ones that are passed to gegl:crop (through g_action_activate).

While the aspect ratio constraint, if any, influences changes to the crop rectangle, it is not the value that's ultimately used for drawing or cropping the image. With this patch, while the final and intermediate aspect ratio values remain separate, the size and position of the crop rectangle don't. So, if g_action_activate is invoked in the middle of an ongoing animation (it's unlikely, but still), then we would end up using the intermediate rectangles to crop the image, and not the final co-ordinates.

Once you follow this train of thought to its logical conclusion, you'll realize that we can also directly animate the (x, y, width, height) of the crop rectangle instead of the aspect ratio. That would require us to animate four properties instead of one, which implies a bit more boiler plate in ToolCropHelper, but we can avoid some arithmetic on each frame because we already know what the  final values of (x, y, width, height) should be, and we won't need a *_visible counterpart of self->crop_aspect_ratio any more.

@@ +933,3 @@
+
+  if (self->active_changed_animation != NULL)
+    egg_animation_stop (self->active_changed_animation);

We should also stop the animation when:

(a) The size of the underlying ImageView changes. This can happen if the window is resized in the middle of an animation.

(b) Deactivating the tool.

(c) If someone tries to resize or move the crop rectangle in the middle of an animation.

All these cases are unlikely unless someone is using EGG_ANIMATION_SLOW_DOWN_FACTOR, but would be good to address to avoid weird corner cases.

@@ +942,3 @@
 photos_tool_crop_landscape_button_toggled (PhotosToolCrop *self)
 {
+  if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->landscape_button)))

Nitpick: Just this part, that avoids updating the constraint twice, could also be part of the patch where you split the callbacks because it is a good thing on its own.

Created attachment 366638 [details] [review]
tool-crop: Don't update the orientation twice when toggled

I took the liberty to make the above adjustments and pushed to master.

Created attachment 366639 [details] [review]
tool-crop: Decouple surface creation and zoomed bbox calculation

Created attachment 366640 [details] [review]
tool-crop: Decouple crop rectangle updates from drawing

Created attachment 366641 [details] [review]
Add PhotosToolCropHelper

Created attachment 366642 [details] [review]
tool-crop: Animate updates to the crop rectangle due to preset changes

Thanks for working on this!