GNOME Bugzilla – Bug 709605
Long press support for touch
Last modified: 2018-03-26 12:31:24 UTC
Would be great if doing a "long" click on the map (and other areas where required) did the same as right-clicking, otherwise these contextual features are inaccessible on touch devices.
Created attachment 271836 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 400ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
Note I originally started with ClutterClickAction, but that introduces more problems than it solves. This works with the existing focus-stealing mechanisms employed inside of gnome-maps, and so continues to work properly. This is tested against gnome-maps 3.10.2, and the diff is against git master, so is suitable for 3.11 series and 3.10 bumps.
Review of attachment 271836 [details] [review]: So, I can't spell.
Created attachment 271839 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 400ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
Updated due to typo (always manage to spell PROPAGATE wrong, and its early morning :))
Review of attachment 271839 [details] [review]: Hi, thanks for the patch! It looks great, I have some nit comments below. I also wonder if this will work along side the solution for bug #724926? Will we be able to implement drag scrolling and still keep this code? ::: src/contextMenu.js @@ +29,3 @@ const Utils = imports.utils; +/* How long before a press becomes a long press ?(>= 400ms) */ Maps uses the C++ style // comments @@ +46,3 @@ this._onButtonReleaseEvent.bind(this)); + /* Expose menu on long presses */ Same comment here @@ +62,3 @@ + case Clutter.EventType.TOUCH_UPDATE: + this._touchTime = Clutter.get_current_event_time(); + break; An empty line between the different cases might make this prettier.
Created attachment 271882 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 400ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
Fixed patch. Well, the update will change the diff time so it wont become a long press if you drag. You're continuously updating the x,y of the device. So you can handle drag somewhere else :) (Note we only consume the event if its a legitimate long press)
(Apologies for not using C++ style comments. I'm a C user daily so JS doesn't come naturally :))
Also, why don't you let me take care of scrolling? Unless you have someone on the case.. I also want to implement pinch zooming, which is fairly possible by using ClutterZoomAction and some hacky maths on the factor :)
Review of attachment 271882 [details] [review]: Thanks for the quick update! I forgot to say something about the commit message. In GNOME commits should link to the bug in bugzilla, there are some proposals on how to do that here: https://wiki.gnome.org/Git/CommitMessages In Maps we use the full link, so it should be: https://bugzilla.gnome.org/review?bug=709605 The Signed-off-by line is not needed. ::: src/contextMenu.js @@ +31,3 @@ +// How long before a press becomes a long press ?(>= 400ms) +const TOUCH_EXPOSE_MENU_TIME = 400; + Maybe we could make this a bit more self documenting, and avoid asking a question in the comment, how about: const LONG_PRESS_TIMEOUT = 400; // msec And about the value, how did you come up with it? I think Android has 500 msec?
(In reply to comment #10) > Also, why don't you let me take care of scrolling? Unless you have someone on > the case.. > I also want to implement pinch zooming, which is fairly possible by using > ClutterZoomAction and some hacky maths on the factor :) Sure, that would be great! :) Is there anyway that you know for us without a fancy touch screen to test this stuff?
(In reply to comment #11) > Review of attachment 271882 [details] [review]: > > Thanks for the quick update! > > I forgot to say something about the commit message. > In GNOME commits should link to the bug in bugzilla, there are some proposals > on how to do that here: https://wiki.gnome.org/Git/CommitMessages > In Maps we use the full link, so it should be: > https://bugzilla.gnome.org/review?bug=709605 Well of course this is wrong, and I can't even blame the early morning. Should be: https://bugzilla.gnome.org/show_bug.cgi?id=709605
Created attachment 271891 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 400ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
...ignore. i changed to 500. tired.
Created attachment 271893 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 500ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
Well, I felt 400 was more natural and responsive than 500ms. That said, I don't always use my touchscreen, so don't take my word for it :) As for testing outside of a touchscreen enabled environment? Not sure I'm afraid :) I know Mattias was going to look at it as he's touch-enabled now (Well. His hardware is.)
(In reply to comment #17) > Well, I felt 400 was more natural and responsive than 500ms. That said, I don't > always use my touchscreen, so don't take my word for it :) > > As for testing outside of a touchscreen enabled environment? Not sure I'm > afraid :) I know Mattias was going to look at it as he's touch-enabled now > (Well. His hardware is.) Sounds good, I'll wait until he gives his ok and then I think this could go in for 3.12. Thanks again!
Thank you Jonas :) My pleasure, I'll take a look at the other touchy-feely changes when I get some more spare time
Some quick observations before getting to sleep. The menu pops up on touch release instead of after those 500ms for me (so if I press the screen for 2 seconds the menu won't show up until I stop pressing). Is this intended? I kind of expected the Android long-press behaviour, but I'm not sure. Also, you can't pan the map via touch in general, but for some reason you can when the context menu is visible. This probably doesn't have with the current bug to do though.
Mattias, that's some voodoo weirdness within Gdk touch events, and grabbing conflicting with the current grab device, so that events from the touch device become button-press-event, etc. Very weird stuff. I can change it to appear before release, it would be a simple case of then checking the x/y didn't change too badly (+- 15px) between touch begin and touch update, and then exposing. Sound ok? I fully expect changes between x/y from begin to update.
Created attachment 272205 [details] Image describing my view on this matter This did not make the 3.11.92 release. My thinking is we try to fix all the touch bugs and include them in the next possible release.
Hah epic. Ok sounds good, do we have a comprehensive list? My understanding on the matter is pan, long press and pinch-zoom are required. I'd personally like to see champlain have some of the touch support, notably pan and pinch zoom. Context menu is gnome-maps specific, though I reckon the rest can be handled inside champlain itself as we're already informed of internal updates via notify::*
(In reply to comment #23) > Hah epic. Ok sounds good, do we have a comprehensive list? > My understanding on the matter is pan, long press and pinch-zoom > are required. > > I'd personally like to see champlain have some of the touch > support, notably pan and pinch zoom. Context menu is gnome-maps > specific, though I reckon the rest can be handled inside champlain > itself as we're already informed of internal updates via notify::* We have a list of sorts, I do not know if it's comprehensive tho. There are three bugs in our bugzilla atm, including this. The other two are: Drag scrolling does not work on tablet https://bugzilla.gnome.org/show_bug.cgi?id=724926 Pinch zoom and rotate touch gestures support https://bugzilla.gnome.org/show_bug.cgi?id=709606 There might be more. I agree with you that these two might be Champlain bugs. And should probably be moved. If you would like to work on them, and others you find might be needed, that would be great. If you think they should be moved to Champlain, feel free to move them. If you do not have sufficient permissions to move them you can comment on them that they should be moved and why, and I will move them. Thanks
Didn't have appropriate permissions, so commented anyway :)
Ping. Is there a new version of this patch in the works Michael?
*** Bug 734243 has been marked as a duplicate of this bug. ***
Anyone want to pick this up?
Created attachment 361368 [details] [review] ContextMenu: Expose context menu on long press When the user initiates a "long press" on their touch screen, we wait until 500ms has passed, before then exposing the context menu. This change is designed to make the ContextMenu more accessible on touch screen enabled devices.
Created attachment 361903 [details] [review] contextMenu: Add a long-press gesture to activate on touch
This patch requires fixes in GTK+ (for the GIR binding), see linked bug.
Created attachment 361914 [details] [review] contextMenu: Add a long-press gesture to activate on touch
-- 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-maps/issues/2.