GNOME Bugzilla – Bug 659693
EOG should have gesture support
Last modified: 2021-06-19 08:48:06 UTC
EOG should enable image manipulation using multi-touch gestures. The attached patch adds this functionality using the libgrip gesture library. The patch is used in the next release of Ubuntu (oneiric). The easiest way to test the feel of it is to download a daily or beta ISO. The patch itself is very strictly #ifdef'd. Building without gesture support should provide a bit-for-bit identical binary to one without this patch.
Created attachment 197152 [details] [review] Patch to add gesture support
Regardless of the contents of the patch (which I haven't bothered to look at yet), it's very kind of you to make a code drop when we are at the edge of the 3.2 release.
Shouldn't this part be #ifdef'd too: + /* pinch gesture data */ + float begin_radius; + float begin_zoom; ?
Created attachment 197213 [details] [review] Patch to add gesture support Fixed missing #ifdef.
I'll have a couple of comments on this in a few days. This week I'm moving my mom to another city, so I can't really review this - but give me until next week and I'll gladly do it. (TL;DR: it's mostly OK; use gdoubles rather than floats for consistency; don't use a static variable; are you sure begin_radius is never 0 and so can break the division)
Created attachment 197454 [details] [review] Updated gesture patch Changed floats to gdoubles. Moved zero_angle to the _EogWindowPrivate struct. Begin_radius is the distance between two touch points when the pinch begins. A zero value would mean that the touches would be on top of each other, which is physically impossible and in any case would be interpreted as a single touch. If you still feel that this should be guarded against, I can fix it.
Review of attachment 197454 [details] [review]: This is all very interesting. Are you planning to support the velocity stuff? (That's how one implements the release-while-dragging-makes-the-image-keep-scrolling behavior, right?) It makes me slightly uncomfortable to have event handlers in both eog-scroll-view and eog-window, although it's probably overkill to have EogScrollView emit an "I need to be rotated" signal... leave it as it is in your patch, but it's something to keep in mind for the future. Also - dragging/zooming have immediate feedback, but rotation is chunky. Is there a common form of feedback for those "you can only rotate in N-degree steps", so the user knows that his gesture is actually doing something? ::: src/eog-scroll-view.c @@ +153,3 @@ + gdouble begin_radius; + gdouble begin_zoom; +#endif The fields for "drag gesture data" have a drag_ prefix. Can you please call the ones for pinching, pinch_* in a similar fashion? This is so we can keep track of which fields are for what among the code. @@ +1696,3 @@ +#ifdef HAVE_LIBGRIP +static void +_utouch_gesture_event (GtkWidget *widget, Our naming convention for callbacks is as follows. If you had a button which emits a clicked signal, the callback would be button_clicked_cb(). Similarly, here please use gesture_event_cb() or similar. @@ +1768,3 @@ + } +} +#endif Hmm, I don't quite like this double-nested pattern. Let's have one function for dragging and another one for pinching. Can you please split things up like static void gesture_event_cb (...) { if (event->type == GRIP_GESTURE_DRAG) handle_drag_gesture (...); else if (event->type == GRIP_GESTURE_PINCH) handle_pinch_gesture (...); else g_assert_not_reached (); } static void handle_drag_gesture (...) { switch (time_type) { case GRIP_TIME_START: ...; case GRIP_TIME_UPDATE: ...; case GRIP_TIME_END: ...; default: g_assert_not_reached (); } } Also, for the GRIP_TIME_END cases, you are just turning off the "dragging" flag. I think you also need to update the last position or rotation (e.g. see eog_scroll_view_button_release_event(), where we *do* update the "last position" based on the button_release event's data). Do you need the test for "if (drag->fingers == 2)"? You are already specifying 2 touch_points to grip_gesture_manager_register_window(). I'm thinking that we now have two ways to initiate/continue/stop a drag; both your patch and the existing eog_scroll_view_button_press_event() (and the other event-y functions) have the same code to deal with the scrolling machinery. It would be good to have a generic start_drag(), update_drag(), stop_drag() set of functions that is called from the gesture and the mouse machinery. What do you think? (It's so little code that it is *not* overkill yet, but since we are updating several struct fields in order to start a drag... it may be good to centralize that in its own function.) ::: src/eog-window.c @@ +106,3 @@ +#ifdef HAVE_LIBGRIP +#define EOG_GESTURE_ROTATE_THRESHOLD 0.785 /* 45 degrees in rad */ You can use M_PI_4 here for clarity. @@ +199,3 @@ #endif +#ifdef HAVE_LIBGRIP + gdouble zero_angle; Call this rotate_start_angle. @@ +4433,3 @@ +#ifdef HAVE_LIBGRIP +static void +_utouch_window_gesture_event (GtkWidget *widget, Same as before, for the naming convention for callbacks. @@ +4474,3 @@ + } +} +#endif Same as before; please handle the GRIP_TIME_END case as well by updating/handling the angles.
Created attachment 197737 [details] [review] Updated gesture patch Due to time issues I have not yet fixed all issues raised. I will add an updated patch some time later, but in the mean time here's an almost fixed one. Velocity has two main parts: detecting flicks and simulating the motion. For the first one we have added flick gestures to the basic stack, but they are not exposed through libgrip yet. The second one is a wider problem. We have started working on a physics based scroller library. See https://wiki.ubuntu.com/Multitouch/Physics. It is not yet finished, though. There have been discussions on having something indicate that a rotation event has been started, such as an overlay graphic. However there is nothing final yet. I admit that it's a bit undiscoverable at the moment. All variables and function names have been renamed as requested. Gesture type based function split is not yet done. I'm not quite sure why you would need to store last locations at GRIP_TIME_END. For rotation the value stored is meaningless if there is no rotation ongoing. Also, AFAIK, the end gestures should have the same values as the last update event had (or, at the very least, the error is less than a few pixels). Could you please clarify why this is needed. The reason that we check for two fingers is that it is left over from older code where we had more finger gestures which we had to temporarily remove. The main issue there is that you want to allow one finger drags on touchscreens. The tricky part is that dragging up with one finger on a touchscreen means "move document upwards" whereas dragging up with two fingers on a touchpad means "move viewport over the document upwards i.e. move the document downwards". This check is in the code as a reminder that it currenly only does two finger drags. It all gets quite complicated but the libgrip tutorial page has an explanation: https://wiki.ubuntu.com/Multitouch/GripTutorial
Created attachment 198325 [details] [review] Updated gesture patch Split the gesture callback function into two.
The biggest problem I have with this at the moment (haven't seen the code yet), is that there is no way for me (us?) to test and provide support for it later on, as I'm missing the required hardware (I guess my Synaptics Touchpad doesn't work with this although it's said to do multitouch stuff). So, if we'd merge it, we'd have to redirect problems with it back to Ubuntu/Launchpad, which feels a bit strange to me.
What concerns me the most is whether we want to add a dependency in this particular library. I think this is something that ideally should be raised at desktop-devel and the usage of libgrip in GNOME must be widely accepted, or at least considered a good way to go. If there are other (GNOME) applications in which libgrip is used (in Ubuntu), it would be good to know and get feedback from the rest of the desktop people. I wouldn't like to depend on this library now to later change it for a different one because every app implements multitouch in a different way.
We have a similar patch for Evince but we have not upstreamed it yet because we wanted to focus on EOG first. The main reason for having libgrip is that GTK proper does not do gestures. AFAIK they are planning to but that will take more than a year.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/eog/-/issues/ Thank you for your understanding and your help.