Created attachment 358973 [details] [review]
Add zoom gesture support for tablets

Please review attached patch which adds zoom gesture support.

Review of attachment 358973 [details] [review]:

Thanks for the patch! Looks very good for a first attempt. The only problem is that the zoom gesture doesn't get disabled along with the rest of zoom controls. That's because the PhotosImageView only implements the zooming mechanism. The policy of when to zoom and by how much is elsewhere.

::: src/photos-image-view.c
@@ +791,3 @@
+photos_image_view_zoom_scale_changed_cb(GtkGestureZoom *gesture, gdouble scale, PhotosImageView *self)
+{
+  photos_image_view_set_zoom(self, scale);

I wonder if we need to disable/tweak our built-in animation between zoom levels. Right now, the image doesn't seem to stick to the fingers when zooming in and out using the gesture.

@@ +813,3 @@
   self->best_fit = TRUE;
+
+  zoom = gtk_gesture_zoom_new(GTK_WIDGET(self));

It will be better to put this in PhotosPreview or PhotosPreviewNavButtons with the rest of the zoom policy logic.

Created attachment 359655 [details] [review]
Add zoom gesture support for tablets v2

Update patch based on your review:

V2:
 - Moved zoom gesture support to photos-preview-nav-buttons
 - Keep track of zoom size
 - Show zoom controls if required

@Debarshi Ray: Do you have time for another review?

Review of attachment 359655 [details] [review]:

I finally got around to trying this on a touch screen. Some comments below:

::: src/photos-preview-nav-buttons.c
@@ +465,3 @@
+
+  self->zoom = self->initial_zoom * scale;
+  photos_preview_view_set_zoom (preview_view, self->zoom);

This doesn't work through the GActions for zooming. That means that all the logic to hide/show the overlaid controls and the minimum zoom threshold don't come into play.

How well does this work with ImageView's built-in animation when zooming in/out? After playing around a bit, I have a feeling that we need to turn off the animation for gestures (by setting the duration to 0).

We might also have to tweak the parameters accepted by our GActions. Currently they are step-based, where 1 step is equivalent to one key press, or one step of the scroll wheel. A zoom factor is then used to actually change the zoom. However, for gestures, we directly get the zoom factor from the gesture as a factor of the initial gesture. Therefore, instead of a simple double, we might need to extend the parameter to be more expressive.

::: src/photos-preview-view.h
@@ +55,3 @@
 void                   photos_preview_view_set_node               (PhotosPreviewView *self, GeglNode *node);
 
+void                   photos_preview_view_set_zoom               (PhotosPreviewView *self, gdouble zoom);

I'd also suggest moving the GtkGestureZoom into PreviewView, where the rest of the zooming interactions are handled.

The PreviewNavButtons is a helper to drive the overlaid controls, and the gestures that it has are there to control their visibility. It'd be nice if we can minimize (or even get rid of) calls to PreviewView from PreviewNavButtons to have a strictly parent to child relationship.

Created attachment 361384 [details] [review]
Make the zoom-in/out GActions more flexible

A proof-of-concept that makes app.zoom-in/out more flexible by making them accept an a{sv} instead of a double. The a{sv} lets us explicitly encode the type of event that triggered the zoom so that the "backed" (ie. the GAction implementation) can handle them accordingly.

Created attachment 361718 [details] [review]
image-view: Split out the code to check valid allocation and extent

Created attachment 361719 [details] [review]
image-view, preview-view: Re-work the zoom animation implementation

Created attachment 361720 [details] [review]
image-view, preview-view: Allow disabling the zoom animation

Created attachment 361722 [details] [review]
Encode the origin of the zoom event in the zoom-in/out GActions

Created attachment 361723 [details] [review]
preview-view: Consolidate zoom calculation from GAction activations

Created attachment 361724 [details] [review]
Prepare the zoom-* GActions for touch gestures

Created attachment 361725 [details] [review]
Add PhotosGestureZoom

Created attachment 361726 [details] [review]
preview-view: Use touch gestures for zooming

Jan, I adjusted your patch on top of the other plumbing changes and pushed to master. Thank you for working on this!