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 770026 - review comments for wip/wayland-tablet-v2
review comments for wip/wayland-tablet-v2
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GdkDevice
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
Carlos Garnacho
Depends on:
Blocks:
 
 
Reported: 2016-08-17 08:32 UTC by Matthias Clasen
Modified: 2016-08-23 19:11 UTC
See Also:
GNOME target: 3.22
GNOME version: ---


Attachments
gdk: Fix GdkDevice::tool-changed signal marshaller (828 bytes, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Fix gdk_device_tool_get_serial() return value (1.16 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Add a getter for the hardware id of a GdkDeviceTool (3.12 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Pass hardware ID on gdk_device_tool_new() (3.00 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
wayland: Add wayland-specific method to retrieve a device node path (1.68 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdkdevice: Add GDK_SOURCE_TABLET_PAD input source type for GdkDevices (1.12 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Add pad event structs, enum values, and event mask bit (6.46 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Address pad events similarly to keyboard events (1.06 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gdk: Add GdkDevicePad (8.60 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gtk: Add minimal handling of pad events (1.47 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
gtk: Add GtkPadController (16.49 KB, patch)
2016-08-17 08:37 UTC, Matthias Clasen
committed Details | Review
wayland: Implement backbone of pad support (15.67 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
wayland: Add GdkWaylandDevicePad (6.69 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
wayland: Create/expose pad devices (1.98 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
wayland: Implement pad event emission (5.93 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
wayland: Support pad devices in gdk_wayland_device_get_node_path() (1000 bytes, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
wayland: Offer wayland-specific method to set pad actions feedback (3.34 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
GtkPadController: Notify actions back to the windowing on wayland (1.98 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review
demos: Add pad support to "event axes" demo (4.71 KB, patch)
2016-08-17 08:38 UTC, Matthias Clasen
committed Details | Review

Description Matthias Clasen 2016-08-17 08:32:19 UTC
I didn't find a bug for this, so I'm just going to file a new one.
Comment 1 Matthias Clasen 2016-08-17 08:37:03 UTC
Created attachment 333451 [details] [review]
gdk: Fix GdkDevice::tool-changed signal marshaller

GdkDeviceTool is an object, not a boxed type.
Comment 2 Matthias Clasen 2016-08-17 08:37:09 UTC
Created attachment 333452 [details] [review]
gdk: Fix gdk_device_tool_get_serial() return value

This is a guint64, not just a guint.
Comment 3 Matthias Clasen 2016-08-17 08:37:13 UTC
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.
Comment 4 Matthias Clasen 2016-08-17 08:37:18 UTC
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.
Comment 5 Matthias Clasen 2016-08-17 08:37:24 UTC
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.
Comment 6 Matthias Clasen 2016-08-17 08:37:29 UTC
Created attachment 333456 [details] [review]
gdkdevice: Add GDK_SOURCE_TABLET_PAD input source type for GdkDevices

This will represent a tablet pad.
Comment 7 Matthias Clasen 2016-08-17 08:37:34 UTC
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.
Comment 8 Matthias Clasen 2016-08-17 08:37:40 UTC
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.
Comment 9 Matthias Clasen 2016-08-17 08:37:45 UTC
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
Comment 10 Matthias Clasen 2016-08-17 08:37:51 UTC
Created attachment 333460 [details] [review]
gtk: Add minimal handling of pad events

No real handling is yet performed, to be done through a GdkEventController
Comment 11 Matthias Clasen 2016-08-17 08:37:56 UTC
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).
Comment 12 Matthias Clasen 2016-08-17 08:38:02 UTC
Created attachment 333462 [details] [review]
wayland: Implement backbone of pad support

All pad interfaces and features are poked, we just now need
exposing those.
Comment 13 Matthias Clasen 2016-08-17 08:38:08 UTC
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.
Comment 14 Matthias Clasen 2016-08-17 08:38:14 UTC
Created attachment 333464 [details] [review]
wayland: Create/expose pad devices
Comment 15 Matthias Clasen 2016-08-17 08:38:20 UTC
Created attachment 333465 [details] [review]
wayland: Implement pad event emission

We now send all the set of button/ring/strip/group_mode events.
Comment 16 Matthias Clasen 2016-08-17 08:38:25 UTC
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.
Comment 17 Matthias Clasen 2016-08-17 08:38:30 UTC
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.
Comment 18 Matthias Clasen 2016-08-17 08:38:38 UTC
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.
Comment 19 Matthias Clasen 2016-08-17 08:38:44 UTC
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.
Comment 20 Matthias Clasen 2016-08-17 08:38:58 UTC
Review of attachment 333451 [details] [review]:

sure
Comment 21 Matthias Clasen 2016-08-17 08:39:40 UTC
Review of attachment 333452 [details] [review]:

sure
Comment 22 Matthias Clasen 2016-08-17 08:42:59 UTC
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
Comment 23 Matthias Clasen 2016-08-17 08:45:04 UTC
Review of attachment 333454 [details] [review]:

ok. will we grow x11 support at some point ?
Comment 24 Matthias Clasen 2016-08-17 08:48:09 UTC
Review of attachment 333455 [details] [review]:

::: gdk/wayland/gdkdevice-wayland.c
@@ +4478,3 @@
+const gchar *
+gdk_wayland_device_get_node_path (GdkDevice *device)
+{

Docs ?
Comment 25 Matthias Clasen 2016-08-17 08:50:06 UTC
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 ?
Comment 26 Matthias Clasen 2016-08-17 08:54:43 UTC
Review of attachment 333457 [details] [review]:

Looks ok
Comment 27 Matthias Clasen 2016-08-17 08:56:56 UTC
Review of attachment 333458 [details] [review]:

ok
Comment 28 Matthias Clasen 2016-08-17 09:00:14 UTC
Review of attachment 333460 [details] [review]:

ok
Comment 29 Matthias Clasen 2016-08-17 09:48:03 UTC
Review of attachment 333462 [details] [review]:

looks ok
Comment 30 Matthias Clasen 2016-08-17 09:56:07 UTC
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.
Comment 31 Matthias Clasen 2016-08-17 09:58:01 UTC
Review of attachment 333464 [details] [review]:

Should probably explain in the commit message how the associations work for these devices.
Comment 32 Matthias Clasen 2016-08-17 10:11:09 UTC
Review of attachment 333465 [details] [review]:

Would be easier to review with documentation for event fields.
Comment 33 Matthias Clasen 2016-08-17 10:12:49 UTC
Review of attachment 333466 [details] [review]:

ok
Comment 34 Matthias Clasen 2016-08-17 10:15:37 UTC
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.
Comment 35 Matthias Clasen 2016-08-17 10:18:15 UTC
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
Comment 36 Matthias Clasen 2016-08-17 10:25:15 UTC
Review of attachment 333469 [details] [review]:

A change of the demo name would be fine  with me.
Comment 37 Matthias Clasen 2016-08-17 10:28:05 UTC
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 ?
Comment 38 Matthias Clasen 2016-08-17 10:29:51 UTC
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
Comment 39 Matthias Clasen 2016-08-17 10:34:19 UTC
I think whats mainly missing here is some highlevel overview documentation to explain how the pad support works, from an application perspective.
Comment 40 Matthias Clasen 2016-08-17 10:36:45 UTC
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 ?
Comment 41 Carlos Garnacho 2016-08-19 13:07:27 UTC
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.
Comment 42 Matthias Clasen 2016-08-20 02:39:30 UTC
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.
Comment 43 Matthias Clasen 2016-08-20 02:57:40 UTC
Also: would be nice to add pad information to the inspector, in the tab where we enumerate input devices and their properties
Comment 44 Matthias Clasen 2016-08-20 04:02:21 UTC
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
Comment 45 Carlos Garnacho 2016-08-20 09:58:15 UTC
(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.
Comment 46 Matthias Clasen 2016-08-20 21:16:10 UTC
(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
Comment 47 Carlos Garnacho 2016-08-21 11:12:33 UTC
(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!
Comment 48 Carlos Garnacho 2016-08-23 19:10:30 UTC
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