GNOME Bugzilla – Bug 771704
Add gesture for switch to previous/next photo
Last modified: 2018-01-23 10:08:06 UTC
.
Created attachment 335918 [details] [review] preview-view: Turn it in a GtkGrid
Created attachment 335919 [details] [review] preview-view: Add a new overlay
Created attachment 335920 [details] [review] preview-view: Move nav buttons to the new overlay
Created attachment 335921 [details] [review] preview-view: Remove double semicolon
Created attachment 335922 [details] [review] preview-view: Introduce swipe gesture
Review of attachment 335921 [details] [review]: Nice catch!
Review of attachment 335918 [details] [review]: ::: src/photos-preview-view.c @@ +73,1 @@ +G_DEFINE_TYPE (PhotosPreviewView, photos_preview_view, GTK_TYPE_GRID); We shouldn't do this. The best practice when creating a new composite widget (ie. a widget made up of other existing widgets) is to derive from GtkBin, unless it is a top-level. eg., https://wiki.gnome.org/HowDoI/CustomWidgets For instance, in the case of PhotosPreviewView, if it is a GtkGrid, then it means that the client code can expect to call any GtkGrid API on it. On the other hand, the exposed API surface is quite limited with GtkBin. Yes, one can use gtk_bin_get_child to mess with the internals, but the API contract says that assuming anything more specific than a GtkWidget is hacky or wrong. You will notice that all current uses of gtk_bin_get_child either (a) work around limitations in gtk+, or (b) limited inside the class itself (eg., photos_preview_view_get_view_from_view_container and photos_tool_filter_button_set_image). The other nice thing about deriving from GtkBin is that it is easier to retain the ABI of the class because you can switch the internal layout from GtkGrid to GtkBox or something else without changing the size of the structs. However, this is mostly important for libraries, not for us.
Review of attachment 335922 [details] [review]: Thanks for the patches, Alessandro. Some comments: ::: src/photos-preview-view.c @@ +582,3 @@ + const char *action_name; + + if (velocity_x == 0) It might be safer to use photos_utils_equal_double instead of a direct comparison. @@ +751,3 @@ + swipe = gtk_gesture_swipe_new (GTK_WIDGET (overlay)); + g_signal_connect (swipe, "swipe", G_CALLBACK (photos_preview_view_swipe_cb), self); Can't we do this inside PhotosPreviewNavButtons? I understand that "buttons" doesn't match with gestures, but it would be nice to keep all the input handling in one place. Maybe we should rename it to "controls" or "input"? But that will destroy the Git history. I don't know. Also, shouldn't this be in the same group as the existing tap or multi-press guesture?
Review of attachment 335919 [details] [review]: ::: src/photos-preview-view.c @@ +706,3 @@ + overlay = gtk_overlay_new (); + gtk_container_add (GTK_CONTAINER (self), overlay); Is there a reason you did this? It does make PreviewView more self-contained, but on the other hand we are already passing the overlay from PhotosEmbed to other classes.
Comment on attachment 335921 [details] [review] preview-view: Remove double semicolon Pushed to master.
Created attachment 358887 [details] [review] Updated patch adding swipe gesture This is an updated patch which focus on the basics and respects the review comments. Please review.
Review of attachment 358887 [details] [review]: I don't understand. There are 4 outstanding patches in the previous patch set but you only submitted one new patch. Are all those 4 other patches obsolete now?
Yes, this patch is all you need to add swipe gesture within photos. I've successfully tested this patch on two tablets. The other patches are not needed.
(In reply to Debarshi Ray from comment #9) > Review of attachment 335919 [details] [review] [review]: > > ::: src/photos-preview-view.c > @@ +706,3 @@ > > + overlay = gtk_overlay_new (); > + gtk_container_add (GTK_CONTAINER (self), overlay); > > Is there a reason you did this? > > It does make PreviewView more self-contained, but on the other hand we are > already passing the overlay from PhotosEmbed to other classes. I did this because of future changes needed for the details sidebar (bug 751116). These overlay changes are necessary because we don't want the navigation buttons floating over the sidebar. These patches (attachment 335919 [details] [review] and 335920) don't actually belong to this bug.
Is the patch ok? Any further comments?
@Debarshi Ray: Is there anything missing in the patch? What are the next steps?
*** Bug 785252 has been marked as a duplicate of this bug. ***
*** Bug 788887 has been marked as a duplicate of this bug. ***
I hadn't realized that this has accumulated so many duplicates. From https://bugzilla.gnome.org/show_bug.cgi?id=785252#c1: The reason I didn't add the swiping is because it needs some GTK+ or widget hacking. Usually what happens with GtkGestureSwipe is that the "swipe" signal is only emitted after the user has completed the sequence and removed the touch point. I'd rather have the content stick to the fingers and only trigger the switch once we have passed a threshold, like it happens on a phone. Otherwise it doesn't feel natural. It could be a fun project for someone with a touch screen and the willingness to do some GTK+ widget/container hacking.
Review of attachment 358887 [details] [review]: Thanks for picking it up. This patch is more or less OK by itself, but we need to look into comment 19 before merging it. Maybe the commit message should also mention Alessandro since this is based on attachment 335922 [details] [review]? ::: src/photos-preview-nav-buttons.c @@ +448,3 @@ + app = g_application_get_default (); + action_name = (velocity_x < 0) ? "load-next" : "load-previous"; + g_action_group_activate_action (G_ACTION_GROUP (app), action_name, NULL); Nitpick. There's already self->load_next/previous. @@ +557,3 @@ + self->swipe_gesture = gtk_gesture_swipe_new (self->preview_view); + g_signal_connect (self->swipe_gesture, "swipe", G_CALLBACK (photos_preview_view_swipe_cb), self); + pan = gtk_gesture_pan_new (self->preview_view, GTK_ORIENTATION_HORIZONTAL); Attachment 335922 [details] had a comment about the pan gesture, but it is not clear to me whether it is really needed. Is it? We don't seem to be claiming or denying the event sequences with it anywhere.
(In reply to Debarshi Ray from comment #20) > @@ +557,3 @@ > + self->swipe_gesture = gtk_gesture_swipe_new (self->preview_view); > + g_signal_connect (self->swipe_gesture, "swipe", G_CALLBACK > (photos_preview_view_swipe_cb), self); > + pan = gtk_gesture_pan_new (self->preview_view, > GTK_ORIENTATION_HORIZONTAL); > > Attachment 335922 [details] had a comment about the pan gesture, but it is > not clear to me whether it is really needed. Is it? We don't seem to be > claiming or denying the event sequences with it anywhere. Or is it bug 788914 in action?
A(In reply to Debarshi Ray from comment #21) > (In reply to Debarshi Ray from comment #20) > > @@ +557,3 @@ > > + self->swipe_gesture = gtk_gesture_swipe_new (self->preview_view); > > + g_signal_connect (self->swipe_gesture, "swipe", G_CALLBACK > > (photos_preview_view_swipe_cb), self); > > + pan = gtk_gesture_pan_new (self->preview_view, > > GTK_ORIENTATION_HORIZONTAL); > > > > Attachment 335922 [details] had a comment about the pan gesture, but it is > > not clear to me whether it is really needed. Is it? We don't seem to be > > claiming or denying the event sequences with it anywhere. > > Or is it bug 788914 in action? Attachment 335922 [details] is mine, I can answer to the question. If I understood correctly, adding gestures to a GtkGestureGroup creates one gesture that is the merging of the gestures added. As the comment says, adding the pan gesture is needed in order to cancel the swipe gesture during non-horizontal panning. I've found this example in the HowDoI/Gesture section here: https://wiki.gnome.org/HowDoI/Gestures In the snippet called "Allowing only horizontal swipes"
Created attachment 361767 [details] [review] pdated patch adding swipe gesture v2 Updated patch based on review comments: V2: - Add x threshold to skip unwanted minimal swipes - Use existing functionality to load previous/next image - Added pan comment
(In reply to Alessandro Bono from comment #22) > Attachment 335922 [details] is mine, I can answer to the question. If I > understood correctly, adding gestures to a GtkGestureGroup creates one > gesture that is the merging of the gestures added. As the comment says, > adding the pan gesture is needed in order to cancel the swipe gesture during > non-horizontal panning. I've found this example in the HowDoI/Gesture > section here: > https://wiki.gnome.org/HowDoI/Gestures > In the snippet called "Allowing only horizontal swipes" Thanks for the explanation, Alessandro. I'll respond in the review below.
Review of attachment 361767 [details] [review]: Thanks for updating the patch! I am speaking from memory because I don't have a touch screen right this moment. Please forgive me, but feel free to say so, if I am mistaken. ::: src/photos-preview-nav-buttons.c @@ +557,3 @@ + self->swipe_gesture = gtk_gesture_swipe_new (self->preview_view); + g_signal_connect (self->swipe_gesture, "swipe", G_CALLBACK (photos_preview_view_swipe_cb), self); Don't we need to set the propagation phase to CAPTURE and touch-only to TRUE? I am worried that since these are single touch gestures, they will be delivered with GdkEventTouch:emulating_pointer=TRUE and cause confusion with the old-style mouse button press/motion/release events in PreviewView that pan/scroll a zoomed in item. See commit fd7bbbb5777c4d04 for an example of this problem that occurred recently. @@ +559,3 @@ + g_signal_connect (self->swipe_gesture, "swipe", G_CALLBACK (photos_preview_view_swipe_cb), self); + /* Ensure only horizontal swipes are delivered */ + pan = gtk_gesture_pan_new (self->preview_view, GTK_ORIENTATION_HORIZONTAL); The detail I was missing is that GtkGesturePan will deny a sequence that locks into the wrong axis. I'd go one step further and also claim the sequence in GtkGesturePan::pan. That will prevent the event sequences from progressing further along the propagation chain. Specifically they won't reach the handlers in PreviewView (see above) and be misinterpreted as something else.
Created attachment 362023 [details] [review] Updated patch adding swipe gesture v3 Updated patch V3: - Set swipe gesture as touch-only and change propagation phase to CAPTURE
You are right. In order to react only on touch screen devices, we need to set it to touch-only. Above patch takes care of it.
@Debarshi: Is this patch ok for an official commit?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-photos/issues/56.