After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734285 - Change defaults to reduce boilerplate in code that uses gestures
Change defaults to reduce boilerplate in code that uses gestures
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkGesture
3.13.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2014-08-05 13:31 UTC by Ray Strode [halfline]
Modified: 2014-08-15 11:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
eventcontroller: Default to GTK_PHASE_BUBBLE (1.39 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
dnd: Set drag gesture propagation phase to GTK_PHASE_NONE explicitly (1.02 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
window: Set multipress gesture phase to be GTK_PHASE_NONE explicitly (1.15 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
gesturesingle: Change default values of button and touch-only properties (3.77 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
colorplane: Update to new GtkGestureSingle/GtkEventController defaults (1.79 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
colorswatch: Update to new GtkGestureSingle/GtkEventController defaults (1.97 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
dnd: Set explicitly GtkGestureSingle::button to 0 on the drag gesture (1.04 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
entry: Update to new GtkGestureSingle/GtkEventController defaults (1.98 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
label: Update to new GtkGestureSingle/GtkEventController defaults (2.11 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
spinbutton: Make the swipe gesture only work on touch devices (1.06 KB, patch)
2014-08-12 13:23 UTC, Carlos Garnacho
committed Details | Review
scrolledwindow: Make kinetic scrolling only work on touch devices (2.47 KB, patch)
2014-08-12 13:24 UTC, Carlos Garnacho
committed Details | Review
window: Set GtkGestureSingle::button to 0 on multipress gesture (1.19 KB, patch)
2014-08-12 13:24 UTC, Carlos Garnacho
committed Details | Review
range: Make multipress gesture accept all buttons (2.88 KB, patch)
2014-08-12 13:24 UTC, Carlos Garnacho
committed Details | Review
textview: Make multipress gesture accept all buttons (2.01 KB, patch)
2014-08-12 13:24 UTC, Carlos Garnacho
committed Details | Review
treeview: Make multipress gesture accept all buttons (3.49 KB, patch)
2014-08-12 13:24 UTC, Carlos Garnacho
committed Details | Review

Description Ray Strode [halfline] 2014-08-05 13:31:27 UTC
I'd like to suggest two changes to the gesture api:

1) set the default propagation phase to "bubble".  The current default is "none" which isn't needed in a lot of common cases.

2) set the default for touch-only in single gestures to "false" and the default button to 1.  swiping or long pressing with a mouse should frequently behave the same way as swiping or long pressing with a finger in my opinion.

By making these changes, users of the gesture api will frequently have less code to write.
Comment 1 Ray Strode [halfline] 2014-08-05 13:53:07 UTC
also 3) gestures should automatically update the event masks of the widgets they're set on
Comment 2 Matthias Clasen 2014-08-06 12:32:04 UTC
sounds plausible to me - Carlos ?
Comment 3 Matthias Clasen 2014-08-09 08:23:44 UTC
If we want to do this, we'll have to land it soon.
Comment 4 Carlos Garnacho 2014-08-12 13:20:07 UTC
Good points Ray, Seeing how the API has evolved I think it makes sense to have such defaults, the initial/current ones being initially thought out as helpers for cases where GtkWidgetClass event handlers were more resilient, but it doesn't really look like a good thing to stick defaults to.

> I'd like to suggest two changes to the gesture api:
> 
> 1) set the default propagation phase to "bubble".  The current default is
> "none" which isn't needed in a lot of common cases.
> 
> 2) set the default for touch-only in single gestures to "false" and the default
> button to 1.  swiping or long pressing with a mouse should frequently behave
> the same way as swiping or long pressing with a finger in my opinion.

I'm attaching a bunch of patches for these ones, with the default value changes, and fixes in places where the older default was assumed. I'm not attaching here any pure cleanup commits yet to avoid cluttering the bug. I think I had the basics covered by following the next hints:

- gtk_event_controller_handle_event() is only called in gtkdnd.c and gtkwindow.c, those places need to set GTK_PHASE_NONE.
- callers of gtk_gesture_single_get_current_button() will need gtk_gesture_single_set_button(gesture,0) now.
- pulling from memory, the only behavior that is exclusively meant for touchscreens and implemented through GtkGesture is scrolledwindow kinetic scrolling and spinbutton swipes.

(In reply to comment #1)
> also 3) gestures should automatically update the event masks of the widgets
> they're set on

I think this is already covered by the patch in https://bugzilla.gnome.org/show_bug.cgi?id=734357#c3
Comment 5 Carlos Garnacho 2014-08-12 13:23:00 UTC
Created attachment 283184 [details] [review]
eventcontroller: Default to GTK_PHASE_BUBBLE

This is in practice the most common value, so make that the default
Comment 6 Carlos Garnacho 2014-08-12 13:23:07 UTC
Created attachment 283185 [details] [review]
dnd: Set drag gesture propagation phase to GTK_PHASE_NONE explicitly

This used to rely on the default phase value.
Comment 7 Carlos Garnacho 2014-08-12 13:23:14 UTC
Created attachment 283186 [details] [review]
window: Set multipress gesture phase to be GTK_PHASE_NONE explicitly

This used to rely on the default value, which has changed to a saner default.
Comment 8 Carlos Garnacho 2014-08-12 13:23:20 UTC
Created attachment 283187 [details] [review]
gesturesingle: Change default values of button and touch-only properties

This now defaults to sane values on mice, so touch-only is set to FALSE,
and button is set to GDK_BUTTON_PRIMARY;
Comment 9 Carlos Garnacho 2014-08-12 13:23:26 UTC
Created attachment 283188 [details] [review]
colorplane: Update to new GtkGestureSingle/GtkEventController defaults

GtkGestureSingle::button is set to 0, as multiple buttons are managed by
the same gesture. Also avoid some extra lines of code setting what nowadays
are default values.
Comment 10 Carlos Garnacho 2014-08-12 13:23:32 UTC
Created attachment 283189 [details] [review]
colorswatch: Update to new GtkGestureSingle/GtkEventController defaults

GtkGestureSingle::button is set to 0 on the multipress gesture, as several
buttons are managed by that gesture. Also avoid some extra lines of code
setting what nowadays are default values.
Comment 11 Carlos Garnacho 2014-08-12 13:23:38 UTC
Created attachment 283190 [details] [review]
dnd: Set explicitly GtkGestureSingle::button to 0 on the drag gesture

This gesture handles drags done with any button, so unset the current button=1
default.
Comment 12 Carlos Garnacho 2014-08-12 13:23:44 UTC
Created attachment 283191 [details] [review]
entry: Update to new GtkGestureSingle/GtkEventController defaults

GtkGestureSingle::button is set to 0 on the drag/multipress gestures, as several
buttons are managed by these gestures. Also, avoid some extra lines of code
setting what nowadays are default values.
Comment 13 Carlos Garnacho 2014-08-12 13:23:51 UTC
Created attachment 283192 [details] [review]
label: Update to new GtkGestureSingle/GtkEventController defaults

GtkGestureSingle::button is set to 0 on the multipress gesture, as several
buttons are managed by that gesture. Also avoid some extra lines of code
setting what nowadays are default values.
Comment 14 Carlos Garnacho 2014-08-12 13:23:57 UTC
Created attachment 283193 [details] [review]
spinbutton: Make the swipe gesture only work on touch devices

This code was relying on the previous GtkGestureSingle::touch-only default
value.
Comment 15 Carlos Garnacho 2014-08-12 13:24:04 UTC
Created attachment 283194 [details] [review]
scrolledwindow: Make kinetic scrolling only work on touch devices

This code was relying on the previous GtkGestureSingle::touch-only default
value, so restore the behavior back.
Comment 16 Carlos Garnacho 2014-08-12 13:24:11 UTC
Created attachment 283195 [details] [review]
window: Set GtkGestureSingle::button to 0 on multipress gesture

This gesture handles events from multiple buttons, so ensure the
gesture still does so after the default value change.
Comment 17 Carlos Garnacho 2014-08-12 13:24:17 UTC
Created attachment 283196 [details] [review]
range: Make multipress gesture accept all buttons

That gesture is meant to handle clicks from several buttons, so unset
the new GDK_BUTTON_PRIMARY default. Also, remove unnecessary boilerplate
with the new default values.
Comment 18 Carlos Garnacho 2014-08-12 13:24:24 UTC
Created attachment 283197 [details] [review]
textview: Make multipress gesture accept all buttons

That gesture is meant to handle clicks on multiple buttons, so unset
the GDK_BUTTON_PRIMARY default. Also, remove unnecessary boilerplate
with the new GtkGestureSingle/GtkEventController defaults.
Comment 19 Carlos Garnacho 2014-08-12 13:24:30 UTC
Created attachment 283198 [details] [review]
treeview: Make multipress gesture accept all buttons

That gesture is meant to handle clicks on multiple buttons, so unset
the GDK_BUTTON_PRIMARY default. Also, remove unnecessary boilerplate
with the new GtkGestureSingle/GtkEventController defaults.
Comment 20 Matthias Clasen 2014-08-13 23:00:53 UTC
Review of attachment 283184 [details] [review]:

ok
Comment 21 Matthias Clasen 2014-08-13 23:01:07 UTC
Review of attachment 283185 [details] [review]:

ok
Comment 22 Matthias Clasen 2014-08-13 23:01:21 UTC
Review of attachment 283186 [details] [review]:

ok
Comment 23 Matthias Clasen 2014-08-13 23:04:48 UTC
Review of attachment 283187 [details] [review]:

ok
Comment 24 Matthias Clasen 2014-08-13 23:05:46 UTC
Review of attachment 283188 [details] [review]:

ok
Comment 25 Matthias Clasen 2014-08-13 23:06:20 UTC
Review of attachment 283189 [details] [review]:

ok
Comment 26 Matthias Clasen 2014-08-13 23:07:30 UTC
Review of attachment 283190 [details] [review]:

could remove the now redundant setting of touch-only to FALSE, here
Comment 27 Matthias Clasen 2014-08-13 23:08:00 UTC
Review of attachment 283191 [details] [review]:

ok
Comment 28 Matthias Clasen 2014-08-13 23:08:25 UTC
Review of attachment 283192 [details] [review]:

ok
Comment 29 Matthias Clasen 2014-08-13 23:08:45 UTC
Review of attachment 283193 [details] [review]:

ok
Comment 30 Matthias Clasen 2014-08-13 23:09:16 UTC
Review of attachment 283194 [details] [review]:

ok
Comment 31 Matthias Clasen 2014-08-13 23:09:35 UTC
Review of attachment 283195 [details] [review]:

ok
Comment 32 Matthias Clasen 2014-08-13 23:10:01 UTC
Review of attachment 283196 [details] [review]:

ok
Comment 33 Matthias Clasen 2014-08-13 23:10:22 UTC
Review of attachment 283197 [details] [review]:

ok
Comment 34 Matthias Clasen 2014-08-13 23:10:40 UTC
Review of attachment 283198 [details] [review]:

ok
Comment 35 Carlos Garnacho 2014-08-15 11:55:32 UTC
Attachment 283184 [details] pushed as a050b01 - eventcontroller: Default to GTK_PHASE_BUBBLE
Attachment 283185 [details] pushed as dd9166c - dnd: Set drag gesture propagation phase to GTK_PHASE_NONE explicitly
Attachment 283186 [details] pushed as 131123c - window: Set multipress gesture phase to be GTK_PHASE_NONE explicitly
Attachment 283187 [details] pushed as 9ad4f1e - gesturesingle: Change default values of button and touch-only properties
Attachment 283188 [details] pushed as 9e89640 - colorplane: Update to new GtkGestureSingle/GtkEventController defaults
Attachment 283189 [details] pushed as 1c23ab5 - colorswatch: Update to new GtkGestureSingle/GtkEventController defaults
Attachment 283190 [details] pushed as 8ca205a - dnd: Set explicitly GtkGestureSingle::button to 0 on the drag gesture
Attachment 283191 [details] pushed as 0c2da16 - entry: Update to new GtkGestureSingle/GtkEventController defaults
Attachment 283192 [details] pushed as 261bbd8 - label: Update to new GtkGestureSingle/GtkEventController defaults
Attachment 283193 [details] pushed as b7412a1 - spinbutton: Make the swipe gesture only work on touch devices
Attachment 283194 [details] pushed as e3a281b - scrolledwindow: Make kinetic scrolling only work on touch devices
Attachment 283195 [details] pushed as c17d3ee - window: Set GtkGestureSingle::button to 0 on multipress gesture
Attachment 283196 [details] pushed as af5a03c - range: Make multipress gesture accept all buttons
Attachment 283197 [details] pushed as c583970 - textview: Make multipress gesture accept all buttons
Attachment 283198 [details] pushed as 91afc0e - treeview: Make multipress gesture accept all buttons