GNOME Bugzilla – Bug 709606
Pinch zoom and rotate touch gestures support
Last modified: 2015-08-30 22:56:24 UTC
Unless my hardware (Thinkpad x201 tablet) is defective/doesn't support multitouch, it appears to me that GNOME Maps doesn't handle pinching the map with two fingers to zoom in/out, or rotating with fingers? Yet another "would be nice to have" feature :)
Definitely. I just got a computer with a touch screen so I now have the possibilty to test this. Will revisit this when the gsoc craze has calmed down a bit.
Request this bug is re-assigned to libchamplain as pinch-zoom support (i.e. via ClutterZoomAction) should be supported widget-side, as opposed to within application logic (i.e. gnome-maps) (I'm unable to do so myself as the bug isn't mine)
Compared to Bug #724926 this one seems to be quite a bit more problematic as zoom levels are currently in multiples of 2 and nothing between (which would be required for pinch-to-zoom). When zoomed by e.g. 1.3 the resulting map and labels get very blurry and quite ugly. Ideally there should be some form of local tile rendering which would render the tiles precisely for the given zoom level.
(In reply to comment #3) > Compared to Bug #724926 this one seems to be quite a bit more problematic as > zoom levels are currently in multiples of 2 and nothing between (which would be > required for pinch-to-zoom). When zoomed by e.g. 1.3 the resulting map and > labels get very blurry and quite ugly. Ideally there should be some form of > local tile rendering which would render the tiles precisely for the given zoom > level. You could also snap to the nearest level. Zooming at 1.3 would snap back to 1.0, zooming to 1.6 would snap at 2.0. The "blurry" tiles would only appear during the pinch animation, which sounds fair enough.
I'm attaching a couple of patches that implement the zoom gesture. I did nothing about fractional zoom levels though, so the visual result is not totally pretty, but at least allows for the interaction to happen. Note that this needs the clutter patches in bug #750496 for libchamplain demos and gnome-maps to work alright.
Created attachment 304707 [details] [review] kinetic-scroll-view: Make it exclusive to pointer/single-touch If we receive touch events, obey a single sequence and cancel all operations if more are received. The return value in the event handlers has been changed so the event is propagated, and kinetic scroll can work in cooperation with pinch/zoom gestures.
Created attachment 304708 [details] [review] view: Implement pinch/zoom gesture Set up a ClutterZoomAction listening for 2-finger touch gestures, and make it take control over over the view position/zoom level. Zooming in/out isn't too smooth yet, because we're constrained to discrete zoom levels (it does animate between these though), this is enough at least for the expected interaction.
Created attachment 304724 [details] [review] view: Implement pinch/zoom gesture Set up a ClutterZoomAction listening for 2-finger touch gestures, and make it take control over over the view position/zoom level. Zooming in/out isn't too smooth yet, because we're constrained to discrete zoom levels (it does animate between these though), this is enough at least for the expected interaction.
Carlos, thanks a lot for working on this! I have no gnome/clutter-powered touchscreen device to test so your work is really welcome. The patches look good in principle (without being able to test myself, I hope they work), I just have a few comments I'll post separately.
Review of attachment 304707 [details] [review]: Please initialize ClutterEventSequence *sequence to NULL in the champlain_kinetic_scroll_view_init () function. In button_press_event_cb () the variables in stage = clutter_actor_get_stage (CLUTTER_ACTOR (scroll)); priv = scroll->priv; are already set at the beginning of the function so you can remove these two lines. I think you can also remove priv->last_motion = 0; because it's initialized every time the motion begins. Add g_signal_emit_by_name (scroll, "panning-completed", NULL) to champlain_kinetic_scroll_view_stop() or call there deceleration_completed_cb() so the signal is called on multitouch when the movement is cancelled. Question: why the change of the return values of the callback functions to FALSE?
Review of attachment 304724 [details] [review]: Maybe I would suggest renaming zoom_began to zoom_started. Please format function parameters of the newly added functions the same way as they are in the rest of the file. In view_find_suitable_zoom () the if checks seem to be redundant - the while loops should be enough.
(In reply to Carlos Garnacho from comment #8) > Created attachment 304724 [details] [review] [review] > view: Implement pinch/zoom gesture > > Set up a ClutterZoomAction listening for 2-finger touch gestures, > and make it take control over over the view position/zoom level. > > Zooming in/out isn't too smooth yet, because we're constrained > to discrete zoom levels (it does animate between these though), > this is enough at least for the expected interaction. The way that it would work, in a lot of UIs, would be: - Blur the view when far enough from the integer zoom levels - When releasing the gesture, snap to the nearest integer zoom level, and start replacing the blurry tiles with their non-fuzzy versions.
The freezes closed in... I'm sorry I dropped the ball here, still want to work on this so it's hopefully ready early on 3.20, managing touchpad gestures too this time.
Hi Carlos, no problem. With the exception of the few minor issues I've pointed out, the patch was fine IMO. I would normally fix them by myself but since I cannot really test this, I wanted you to update the patch. I think it's better to have at least some support of touchscreens than none so if the patch improves the situation at least a bit, I'd like to merge it - it can always be improved later. Would you find some time to update the patches?
Created attachment 309859 [details] [review] kinetic-scroll-view: Make it exclusive to pointer/single-touch If we receive touch events, obey a single sequence and cancel all operations if more are received. The return value in the event handlers has been changed so the event is propagated, and kinetic scroll can work in cooperation with pinch/zoom gestures.
Created attachment 309860 [details] [review] view: Implement pinch/zoom gesture Set up a ClutterZoomAction listening for 2-finger touch gestures, and make it take control over over the view position/zoom level. Zooming in/out isn't too smooth yet, because we're constrained to discrete zoom levels (it does animate between these though), this is enough at least for the expected interaction.
Cheers jiri, I do think too that this is better than nothing, might be worth requesting a freeze break :), some replies to your review below: (In reply to Jiri Techet from comment #10) > Review of attachment 304707 [details] [review] [review]: > > Please initialize ClutterEventSequence *sequence to NULL in the > champlain_kinetic_scroll_view_init () function. > > In button_press_event_cb () the variables in > > stage = clutter_actor_get_stage (CLUTTER_ACTOR (scroll)); > priv = scroll->priv; > > are already set at the beginning of the function so you can remove these two > lines. I think you can also remove > > priv->last_motion = 0; > > because it's initialized every time the motion begins. Right, removed these. > > Add g_signal_emit_by_name (scroll, "panning-completed", NULL) to > champlain_kinetic_scroll_view_stop() or call there > deceleration_completed_cb() so the signal is called on multitouch when the > movement is cancelled. Right, added too in the patch. > > Question: why the change of the return values of the callback functions to > FALSE? This is due to the way gestures connect to the stage in order to receive events, we must let these events through or the touch management in the ClutterSwipeAction will be botched... Added notes on the updated patch explaining the situation. (In reply to Jiri Techet from comment #11) > Review of attachment 304724 [details] [review] [review]: > > Maybe I would suggest renaming zoom_began to zoom_started. Done :) > > Please format function parameters of the newly added functions the same way > as they are in the rest of the file. Oops, missed that formatting detail, done now. > > In view_find_suitable_zoom () the if checks seem to be redundant - the while > loops should be enough. Hmm, that's right, although slightly less readable IMO, done anyway.
(In reply to Carlos Garnacho from comment #13) > The freezes closed in... I'm sorry I dropped the ball here, still want to > work on this so it's hopefully ready early on 3.20, managing touchpad > gestures too this time. You should totally ask for a freeze break.
(In reply to Matthias Clasen from comment #18) > (In reply to Carlos Garnacho from comment #13) > > The freezes closed in... I'm sorry I dropped the ball here, still want to > > work on this so it's hopefully ready early on 3.20, managing touchpad > > gestures too this time. > > You should totally ask for a freeze break. Went ahead and sent https://mail.gnome.org/archives/release-team/2015-August/msg00029.html, hope not to step on your toes Jiri :).
Just for my information - how is it with freezes of modules from gnome-suites-core-deps (used to be gnome-external-deps) where libchamplain is? I have been under the impression these are external to gnome and don't have to follow the freezes so rigidly and releases can be kind of independent of gnome releases (there's e.g. WebKit here which I think doesn't follow the gnome release cycle). Has anything changed in this respect?
Merged, thanks. I just made some minor fixes - you may want to have a look at them but they are so trivial I haven't screwed up anything.