GNOME Bugzilla – Bug 770026
review comments for wip/wayland-tablet-v2
Last modified: 2016-08-23 19:11:54 UTC
I didn't find a bug for this, so I'm just going to file a new one.
Created attachment 333451 [details] [review] gdk: Fix GdkDevice::tool-changed signal marshaller GdkDeviceTool is an object, not a boxed type.
Created attachment 333452 [details] [review] gdk: Fix gdk_device_tool_get_serial() return value This is a guint64, not just a guint.
Created attachment 333453 [details] [review] gdk: Add a getter for the hardware id of a GdkDeviceTool Although scarcely used, this information may be useful to retrieve from the windowing systems that offer this information.
Created attachment 333454 [details] [review] gdk: Pass hardware ID on gdk_device_tool_new() And implement this on wayland, where this information is already obtained.
Created attachment 333455 [details] [review] wayland: Add wayland-specific method to retrieve a device node path This will be useful at least for g-c-c, in order to match libwacom data with GdkDevices.
Created attachment 333456 [details] [review] gdkdevice: Add GDK_SOURCE_TABLET_PAD input source type for GdkDevices This will represent a tablet pad.
Created attachment 333457 [details] [review] gdk: Add pad event structs, enum values, and event mask bit GDK_PAD_BUTTON*,RING and STRIP will be emitted respectively when pad buttons, rings or strips are interacted with. Each of those pad components belong to a group (a pad can contain several of those), which may be in a given mode. All this information is contained in the event. GDK_PAD_GROUP_MODE is emitted when a group in the pad switches mode, which will generally result in a different set of actions being triggered from the same buttons/rings/strips in the group.
Created attachment 333458 [details] [review] gdk: Address pad events similarly to keyboard events We want the same treatment for those, the event will be emitted on the toplevel, which will then decide what to do with the event. It just doesn't make much sense to propagate those up/down the hierarchy, when we want specifically one action being triggered from those.
Created attachment 333459 [details] [review] gdk: Add GdkDevicePad This is an interface meant to be implemented by the "pad" devices. This device-specific interface exposes the mapping of all pad features, it allows retrieving: - The number of buttons/rings/strips - The number of groups - The number of modes a group has - Whether a given button/ring/strip belongs to a given group
Created attachment 333460 [details] [review] gtk: Add minimal handling of pad events No real handling is yet performed, to be done through a GdkEventController
Created attachment 333461 [details] [review] gtk: Add GtkPadController This GdkEventController is a helper object to handle pad events, it allows setting a mapping to action names, to be triggered in the given action group. In order to help on places where advanced mapping/configurability of pad features is not desirable, this controller also allows passing a NULL pad device, meaning it will listen on all pads, and/or passing -1 on mode/index, so an action applies to all modes/features (eg. strips/rings).
Created attachment 333462 [details] [review] wayland: Implement backbone of pad support All pad interfaces and features are poked, we just now need exposing those.
Created attachment 333463 [details] [review] wayland: Add GdkWaylandDevicePad This is a subclass of GdkWaylandDevice that implements GdkDevicePad, all pad features are looked up from the info obtained through the tablet v2 interface.
Created attachment 333464 [details] [review] wayland: Create/expose pad devices
Created attachment 333465 [details] [review] wayland: Implement pad event emission We now send all the set of button/ring/strip/group_mode events.
Created attachment 333466 [details] [review] wayland: Support pad devices in gdk_wayland_device_get_node_path() We can return the node path on those too, so do that.
Created attachment 333467 [details] [review] wayland: Offer wayland-specific method to set pad actions feedback The wayland tablet protocol allows notifying the compositor with descriptions of the actions performed by each tablet element. This API call allows to hook up in to this wayland-specific feature.
Created attachment 333468 [details] [review] GtkPadController: Notify actions back to the windowing on wayland This allows applications to provide descriptions of the actions performed by each pad feature. pass the GtkPadActionEntry labels for this.
Created attachment 333469 [details] [review] demos: Add pad support to "event axes" demo More of an "advanced device features" demo now, probably worth pondering a name change. As for pad support in the demo, just keep it "simple", make the controller handle all pad devices, and make all the actions have the same callback.
Review of attachment 333451 [details] [review]: sure
Review of attachment 333452 [details] [review]: sure
Review of attachment 333453 [details] [review]: ::: gdk/gdkdevicetool.c @@ +172,3 @@ + * + * Gets the hardware ID of this tool, or 0 if it's not known. + * I think we should document some of the guarantees or intended uses here: - ideally unique - will not change for the same physical hw - can be used to persistently store device-specific configuration
Review of attachment 333454 [details] [review]: ok. will we grow x11 support at some point ?
Review of attachment 333455 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +4478,3 @@ +const gchar * +gdk_wayland_device_get_node_path (GdkDevice *device) +{ Docs ?
Review of attachment 333456 [details] [review]: ::: gdk/gdkdevice.h @@ +51,3 @@ * added in 3.22 + * @GDK_SOURCE_TABLET_PAD: the device is a "pad", a collection of buttons, + * rings and strips. This device type has been added in 3.22. Should probably be a bit more concrete here: these pads are basically always the 'buttons' part of a graphics tablet, right ?
Review of attachment 333457 [details] [review]: Looks ok
Review of attachment 333458 [details] [review]: ok
Review of attachment 333460 [details] [review]: ok
Review of attachment 333462 [details] [review]: looks ok
Review of attachment 333463 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +901,3 @@ + return -1; + + return group->n_modes; The n's are confusing me here. This is a getter for the number of modes of a certain group, right ? I would suggest to name the argument group_idx, or so, and maybe rename the function to gdk_wayland_device_pad_group_get_n_modes @@ +907,3 @@ +gdk_wayland_device_pad_get_n_features (GdkDevicePad *pad, + GdkDevicePadFeature feature) +{ Similar confusion here: this is returning the number of features of a certain kind ? Perhaps not as bad as the previous one. @@ +932,3 @@ + GdkDevicePadFeature feature, + gint idx) +{ What does this return ? I can't really make it out from the code.
Review of attachment 333464 [details] [review]: Should probably explain in the commit message how the associations work for these devices.
Review of attachment 333465 [details] [review]: Would be easier to review with documentation for event fields.
Review of attachment 333466 [details] [review]: ok
Review of attachment 333467 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +5188,3 @@ +void +gdk_wayland_device_pad_set_feedback (GdkDevice *device, + GdkDevicePadFeature element, In other places, you've called GdkDevicePadFeature arguments 'feature', I think @@ +5191,3 @@ + guint idx, + const gchar *label) +{ Documentation would be really useful to understand what this does.
Review of attachment 333468 [details] [review]: ::: gtk/gtkpadcontroller.c @@ +140,3 @@ + + gdk_wayland_device_pad_set_feedback (pad, elem, idx, + g_dgettext (NULL, entry->label)); NULL ? That doesn't look right
Review of attachment 333469 [details] [review]: A change of the demo name would be fine with me.
Review of attachment 333461 [details] [review]: ::: gtk/gtkpadcontroller.c @@ +136,3 @@ + return FALSE; +} + I think filter_event needs some better documentation in GtkEventController - what do the TRUE and FALSE return values here indicate ? ::: gtk/gtkpadcontroller.h @@ +52,3 @@ + gint mode; + gchar *label; + gchar *action_name; Do we need to allow actions with parameters ? If so, this needs to be a detailed_action_name, or (better) store the param gvariant separately. @@ +65,3 @@ +GDK_AVAILABLE_IN_3_22 +void gtk_pad_controller_set_action_group (GtkPadController *controller, + GActionGroup *group); Do we expect there to be only a single pad controller per application ? Do we we expect all pad actions to be in a single action group ?
Review of attachment 333459 [details] [review]: ::: gdk/gdkdevicepad.h @@ +55,3 @@ +GDK_AVAILABLE_IN_3_22 +gint gdk_device_pad_get_n_features (GdkDevicePad *pad, + GdkDevicePadFeature feature); I have some comments in the patch adding the wayland implementation of the api
I think whats mainly missing here is some highlevel overview documentation to explain how the pad support works, from an application perspective.
Review of attachment 333467 [details] [review]: how is this going to work ? every application has its own set of action labels. When are these shown ? does the compositor determine which labels to show dependending on the currently active app ?
Thanks so much for the review! Agreed docs were in a poor state... I made it up now in the branch. (In reply to Matthias Clasen from comment #22) > Review of attachment 333453 [details] [review] [review]: > > ::: gdk/gdkdevicetool.c > @@ +172,3 @@ > + * > + * Gets the hardware ID of this tool, or 0 if it's not known. > + * > > I think we should document some of the guarantees or intended uses here: > > - ideally unique > - will not change for the same physical hw > - can be used to persistently store device-specific configuration I've added docs for this. It might not be as unique as you think though, two identical styli will have the same hardware ID, but different serials. Still, this is more concrete than the GdkDeviceToolType enum (pen, eraser, airbrush,...) because one tablet may support styli with different hardware IDs, but with the same tool type. This is mostly so we can look things up properly in libwacom. (In reply to Matthias Clasen from comment #23) > Review of attachment 333454 [details] [review] [review]: > > ok. will we grow x11 support at some point ? Optimistically yes. This info is obtained from libinput in the compositor side, xf86-input-libinput might export this as a device property as well. (In reply to Matthias Clasen from comment #25) > Review of attachment 333456 [details] [review] [review]: > > ::: gdk/gdkdevice.h > @@ +51,3 @@ > * added in 3.22 > + * @GDK_SOURCE_TABLET_PAD: the device is a "pad", a collection of buttons, > + * rings and strips. This device type has been added in 3.22. > > Should probably be a bit more concrete here: these pads are basically always > the 'buttons' part of a graphics tablet, right ? You're right, made that more concrete. (In reply to Matthias Clasen from comment #30) > Review of attachment 333463 [details] [review] [review]: > > ::: gdk/wayland/gdkdevice-wayland.c > @@ +901,3 @@ > + return -1; > + > + return group->n_modes; > > The n's are confusing me here. This is a getter for the number of modes of a > certain group, right ? > I would suggest to name the argument group_idx, or so, and maybe rename the > function to > gdk_wayland_device_pad_group_get_n_modes Indeed, I've renamed the function as you suggested, and added some explanations on GdkDevicePad docs. > > @@ +907,3 @@ > +gdk_wayland_device_pad_get_n_features (GdkDevicePad *pad, > + GdkDevicePadFeature feature) > +{ > > Similar confusion here: this is returning the number of features of a > certain kind ? > Perhaps not as bad as the previous one. Yes, that's the case. I guess I might better rename the enum to GdkDevicePadFeatureType or so, although strikes me as too long. > > @@ +932,3 @@ > + GdkDevicePadFeature feature, > + gint idx) > +{ > > What does this return ? I can't really make it out from the code. The gist of this whole API is: pads may divide their buttons/rings/strips into groups, each containing a subset of these features. they also have "modes" that may end up affecting the final result of pressing a button (might be seen as modifiers in the keyboard). with this API you can query the number of buttons/rings/strips, the number of groups, the number of modes per group, and the group each of these buttons/rings/strips belong to. As a practical example, see https://wacom.com/~/media/images/products/pen-displays/cintiq-24hd-touch/cintiq-24hd-productivity.png . The cintiq 24HD has these sets of buttons on both sides of the tablet, so it has two groups, each containing 1 ring and 8(ish) buttons. The 3 [-] buttons beside the ring are actually "mode switch" buttons (the small line in the button is a led, telling which was last pressed), they would be consumed by the compositor to end up emitting GdkEventPadGroupMode, so this tablet would effectively have 3 modes per group which allow mapping 3 different actions to the ring and 5 remaining buttons in each group. This is the summit of overconfigurability, I admit :P. (In reply to Matthias Clasen from comment #31) > Review of attachment 333464 [details] [review] [review]: > > Should probably explain in the commit message how the associations work for > these devices. Oops, indeed... added some proper commit log. (In reply to Matthias Clasen from comment #34) > Review of attachment 333467 [details] [review] [review]: > > ::: gdk/wayland/gdkdevice-wayland.c > @@ +5188,3 @@ > +void > +gdk_wayland_device_pad_set_feedback (GdkDevice *device, > + GdkDevicePadFeature element, > > In other places, you've called GdkDevicePadFeature arguments 'feature', I > think Right, renamed it here, and "idx" below to feature_idx. (In reply to Matthias Clasen from comment #35) > Review of attachment 333468 [details] [review] [review]: > > ::: gtk/gtkpadcontroller.c > @@ +140,3 @@ > + > + gdk_wayland_device_pad_set_feedback (pad, elem, idx, > + g_dgettext (NULL, > entry->label)); > > NULL ? That doesn't look right Hmm, might have been me misreading g_dgettext() docs, but I expect these strings to always be defined by the application (ie. the caller of textdomain()). I did here more or less what GtkActionGroup used to do for GtkActionEntry labels previously marked as translatable through N_(). I guessed that the usecase of having strings here provided by libraries is more strange, so gtk_pad_controller_set_translation_domain() seemed more moot to me, would you feel more comfortable with it? Another option is getting rid of GtkPadActionEntry entirely and just require already translated strings in gtk_pad_controller_set_action(), the entry helpers will likely be unused as soon as there's some application configurability over pad actions, although seems like a neat helper for applications that might want to offer some basic, generic support. (In reply to Matthias Clasen from comment #36) > Review of attachment 333469 [details] [review] [review]: > > A change of the demo name would be fine with me. Done :)(In reply to Matthias Clasen from comment #37) > Review of attachment 333461 [details] [review] [review]: > > ::: gtk/gtkpadcontroller.c > @@ +136,3 @@ > + return FALSE; > +} > + > > I think filter_event needs some better documentation in GtkEventController - > what do the TRUE and FALSE return values here indicate ? Agreed, will deal with this as a separate patch. TRUE means "event is filtered", so ::handle-event won't trigger with those. > > ::: gtk/gtkpadcontroller.h > @@ +52,3 @@ > + gint mode; > + gchar *label; > + gchar *action_name; > > Do we need to allow actions with parameters ? If so, this needs to be a > detailed_action_name, or (better) store the param gvariant separately. Yes, I agree it's better to store separately from the static GtkPadActionEntry info, maybe a point in favor for removal. > > @@ +65,3 @@ > +GDK_AVAILABLE_IN_3_22 > +void gtk_pad_controller_set_action_group (GtkPadController *controller, > + GActionGroup *group); > > Do we expect there to be only a single pad controller per application ? Do > we we expect all pad actions to be in a single action group ? (FWIW, that function definition was a leftover, the action group is construct only, it's removed now) There may be multiple pad controllers per toplevel, up to one per pad device plugged, although I decided to allow a NULL device so we may have one controller for all pads for the simplest cases. For the most complex usecases, I expect apps to create and populate those with actions from their own config on the fly when pad devices are plugged/detected, and tear those controllers down when the device is unplugged. I didn't come to think too hard about multiple action groups though, what I'd expect however is that all pad controllers share the same set of possible actions (eg. those that can be mapped from the application configuration dialog), so they'd be most likely pointing to the same action group. (In reply to Matthias Clasen from comment #39) > I think whats mainly missing here is some highlevel overview documentation > to explain how the pad support works, from an application perspective. I hope it's better now in the branch :) (In reply to Matthias Clasen from comment #40) > Review of attachment 333467 [details] [review] [review]: > > how is this going to work ? every application has its own set of action > labels. When are these shown ? does the compositor determine which labels to > show dependending on the currently active app ? FTR, yes, this is all redirected to the pad OSD, so it gives context-aware info.
Thanks for documenting. You were right about g_dgettext(). The remaining question I have now is: can we implement this on other backends ? Currently, it appears to be a bunch of backend-neutral api is added for a Wayland-only feature.
Also: would be nice to add pad information to the inspector, in the tab where we enumerate input devices and their properties
Attachment 333451 [details] pushed as 7e11fca - gdk: Fix GdkDevice::tool-changed signal marshaller Attachment 333452 [details] pushed as e3bbeb4 - gdk: Fix gdk_device_tool_get_serial() return value
(In reply to Matthias Clasen from comment #42) > Thanks for documenting. You were right about g_dgettext(). > > The remaining question I have now is: can we implement this on other > backends ? Currently, it appears to be a bunch of backend-neutral api is > added for a Wayland-only feature. Tbf, I expect the only wayland-specific feature to be the feedback string. Although support for pad controllers might be a bit spotty. Admittedly x11 won't be straightforward to adapt to this, mainly because pads look enough like a pointer to have these devices be a slave of the master pointer, thus their focus follows the mouse. Probably the best we can do there is keep going with session-level passive grab on pad buttons, and doing button->keycombo translations, just like it used to work. I also thought at some point of making clients on x11 attempt to passive grab pad buttons on focus in, so we can deal with those regardless of the mouse focus, sounds icky (esp. dealing with unsuccessful grabs around focus changes) but maybe worth trying at some point, or we could just give up on the keyboard-like focus semantics and deem the "focus follows pointer" as a platform-specific detail. As for other backends, I know at least OSX has its kind-of OSD (called HUD there), and can leave the choice of the actions to perform to the application, just that these application-defined buttons will appear as "Application defined" in the OSD. I think it's feasible there to support pad events and GtkPadController. I however don't know whether this is possible in windows, I *think* only global actions are possible there, so client-side handling is not possible. (In reply to Matthias Clasen from comment #43) > Also: would be nice to add pad information to the inspector, in the tab > where we enumerate input devices and their properties Indeed, looking into that next. I also thought might be nice at some point (although further extra UI) to have these actions in GtkShortcutsWindow, I'd rather not drag the libwacom/rsvg tinkering here though.
(In reply to Carlos Garnacho from comment #45) > (In reply to Matthias Clasen from comment #42) > > Thanks for documenting. You were right about g_dgettext(). > > > > The remaining question I have now is: can we implement this on other > > backends ? Currently, it appears to be a bunch of backend-neutral api is > > added for a Wayland-only feature. > > Tbf, I expect the only wayland-specific feature to be the feedback string. > Although support for pad controllers might be a bit spotty. > > Admittedly x11 won't be straightforward to adapt to this, mainly because > pads look enough like a pointer to have these devices be a slave of the > master pointer, thus their focus follows the mouse. Probably the best we can > do there is keep going with session-level passive grab on pad buttons, and > doing button->keycombo translations, just like it used to work. > > I also thought at some point of making clients on x11 attempt to passive > grab pad buttons on focus in, so we can deal with those regardless of the > mouse focus, sounds icky (esp. dealing with unsuccessful grabs around focus > changes) but maybe worth trying at some point, or we could just give up on > the keyboard-like focus semantics and deem the "focus follows pointer" as a > platform-specific detail. That seems like an ok idea (to treat the focus behavior as platform-specific). > As for other backends, I know at least OSX has its kind-of OSD (called HUD > there), and can leave the choice of the actions to perform to the > application, just that these application-defined buttons will appear as > "Application defined" in the OSD. I think it's feasible there to support pad > events and GtkPadController. > > I however don't know whether this is possible in windows, I *think* only > global actions are possible there, so client-side handling is not possible. To turn this around a bit: would it be worthwhile/possible to have global, predefined default actions ? Or does that not make sense ? > (In reply to Matthias Clasen from comment #43) > > Also: would be nice to add pad information to the inspector, in the tab > > where we enumerate input devices and their properties > > Indeed, looking into that next. I also thought might be nice at some point > (although further extra UI) to have these actions in GtkShortcutsWindow, I'd > rather not drag the libwacom/rsvg tinkering here though. Yes to both. I would be open to treating both the inspector and the shortcutswindow as separate follow ups, and land the core functionality now, so we have the apis in place for 3.22
(In reply to Matthias Clasen from comment #46) > (In reply to Carlos Garnacho from comment #45) > > (In reply to Matthias Clasen from comment #42) > > > Thanks for documenting. You were right about g_dgettext(). > > > > > > The remaining question I have now is: can we implement this on other > > > backends ? Currently, it appears to be a bunch of backend-neutral api is > > > added for a Wayland-only feature. > > > > Tbf, I expect the only wayland-specific feature to be the feedback string. > > Although support for pad controllers might be a bit spotty. > > > > Admittedly x11 won't be straightforward to adapt to this, mainly because > > pads look enough like a pointer to have these devices be a slave of the > > master pointer, thus their focus follows the mouse. Probably the best we can > > do there is keep going with session-level passive grab on pad buttons, and > > doing button->keycombo translations, just like it used to work. > > > > I also thought at some point of making clients on x11 attempt to passive > > grab pad buttons on focus in, so we can deal with those regardless of the > > mouse focus, sounds icky (esp. dealing with unsuccessful grabs around focus > > changes) but maybe worth trying at some point, or we could just give up on > > the keyboard-like focus semantics and deem the "focus follows pointer" as a > > platform-specific detail. > > That seems like an ok idea (to treat the focus behavior as > platform-specific). I'll play around with it :). > > > As for other backends, I know at least OSX has its kind-of OSD (called HUD > > there), and can leave the choice of the actions to perform to the > > application, just that these application-defined buttons will appear as > > "Application defined" in the OSD. I think it's feasible there to support pad > > events and GtkPadController. > > > > I however don't know whether this is possible in windows, I *think* only > > global actions are possible there, so client-side handling is not possible. > > To turn this around a bit: would it be worthwhile/possible to have global, > predefined default actions ? Or does that not make sense ? I tbh shied away from this approach. Basically global actions imply translations to keyboard shortcuts in all platforms (we're the only one which doesn't allow per-app mappings though), and those get non-standard, are potentially affected by key themes/i18n, etc... For wayland there *might* have been a more semantic set of global actions, but there's still concerns about how widely useful or future-proof those are, so felt better to defer the actions to clients. That said, we still allow this mapping to keyboard shortcuts in mutter/g-s, those buttons would just be unseen by the client. > > > (In reply to Matthias Clasen from comment #43) > > > Also: would be nice to add pad information to the inspector, in the tab > > > where we enumerate input devices and their properties > > > > Indeed, looking into that next. I also thought might be nice at some point > > (although further extra UI) to have these actions in GtkShortcutsWindow, I'd > > rather not drag the libwacom/rsvg tinkering here though. > > Yes to both. I would be open to treating both the inspector and the > shortcutswindow as separate follow ups, and land the core functionality now, > so we have the apis in place for 3.22 Sounds good!
Attachment 333453 [details] pushed as 40f75e7 - gdk: Add a getter for the hardware id of a GdkDeviceTool Attachment 333454 [details] pushed as 942d144 - gdk: Pass hardware ID on gdk_device_tool_new() Attachment 333455 [details] pushed as 3ac56e6 - wayland: Add wayland-specific method to retrieve a device node path Attachment 333456 [details] pushed as 3f56af3 - gdkdevice: Add GDK_SOURCE_TABLET_PAD input source type for GdkDevices Attachment 333457 [details] pushed as 0dcb9b3 - gdk: Add pad event structs, enum values, and event mask bit Attachment 333458 [details] pushed as f1a9cd4 - gdk: Address pad events similarly to keyboard events Attachment 333459 [details] pushed as b8a77d4 - gdk: Add GdkDevicePad Attachment 333460 [details] pushed as 3a6d0ff - gtk: Add minimal handling of pad events Attachment 333461 [details] pushed as a0b9586 - gtk: Add GtkPadController Attachment 333462 [details] pushed as feb09e3 - wayland: Implement backbone of pad support Attachment 333463 [details] pushed as 82a46fa - wayland: Add GdkWaylandDevicePad Attachment 333464 [details] pushed as cca51b7 - wayland: Create/expose pad devices Attachment 333465 [details] pushed as 7e961b8 - wayland: Implement pad event emission Attachment 333466 [details] pushed as 27f879b - wayland: Support pad devices in gdk_wayland_device_get_node_path() Attachment 333467 [details] pushed as 87af999 - wayland: Offer wayland-specific method to set pad actions feedback Attachment 333468 [details] pushed as e66a821 - GtkPadController: Notify actions back to the windowing on wayland Attachment 333469 [details] pushed as ae29157 - demos: Add pad support to "event axes" demo