Created attachment 354974 [details] [review]
backends: Map tablet pad rings/strips to action settings

Just like we do for buttons, with a few twists. These have 2 directions
mappable to different keycombos, and are affected by the current mode
in their group.

Created attachment 354975 [details] [review]
backends: Extend pad action label checks to rings/strips

This way the pad OSD can obtain the keycombos that are mapped to these
for labeling purposes.

Created attachment 354976 [details] [review]
clutter/x11: Emit CLUTTER_PAD_RING/STRIP events on X11

These events will be useful on gnome-shell UI, so translate the
4-5 button events with exotic axes to those.

Created attachment 354977 [details] [review]
padOsd: Refactor function setting label after edition into separatte function

Created attachment 354978 [details] [review]
padOsd: Implement edition of actions for rings/strips

Customization of keycombo actions for strips/rings was lost in the
porting to new incarnation of Wacom support.

The UI here is slightly different, instead of requiring the user to
rotate/swipe in each direction to map each keycombo, the UI will
navigate the user through edition of both options, first one, then
the other.

Also, currently just maps actions on mode=0, mapping actions on
different modes require Clutter API additions that will be added
in future commits.

A few problems with this set of commits:

  - "clutter/x11: Emit CLUTTER_PAD_RING/STRIP events on X11"
      * `valuator->mode = XIModeAbsolute` should have `==`

  - The "Map Buttons..." button doesn't appear under X11 (but does under Wayland)

  - Clicking "Map Buttons..." for my Cintiq 24HDT causes GNOME to crash

  - Clicking "Map Buttons..." for my Intuos Pro 2 works, but...
      * The list of options available to the ring includes more than keystrokes
      * Mapping the ring to a keystroke (keypad '5') doesn't send the right events

I can't do much more with these for the time being since I'll be out of the office until next week. I'll catch up with this bug then :)

Created attachment 355464 [details] [review]
backends: Extend pad action label checks to rings/strips

This way the pad OSD can obtain the keycombos that are mapped to these
for labeling purposes.

Created attachment 355465 [details] [review]
padOsd: Refactor function setting label after edition into separate function

Created attachment 355466 [details] [review]
padOsd: Implement edition of actions for rings/strips

Customization of keycombo actions for strips/rings was lost in the
porting to new incarnation of Wacom support.

The UI here is slightly different, instead of requiring the user to
rotate/swipe in each direction to map each keycombo, the UI will
navigate the user through edition of both options, first one, then
the other.

Also, currently just maps actions on mode=0, mapping actions on
different modes require Clutter API additions that will be added
in future commits.

Created attachment 355467 [details] [review]
padOsd: Disallow help/switch monitor actions on rings/strips

It does not make sense to map those actions to non-buttons. Set the
actions insensitive in the combobox to disallow this from the UI.

Created attachment 355468 [details] [review]
padOsd: strengthen ring/strip label creation

If the padOsd is given a nonexistent ring/strip, things would fail
badly later when trying to paint a 0x0 StLabel. Just avoid creating
more ring/strip labels than those known by libwacom.

This is unlikely to happen, but seems better to protect against it.

(In reply to Jason Gerecke from comment #6)
> A few problems with this set of commits:
> 
>   - "clutter/x11: Emit CLUTTER_PAD_RING/STRIP events on X11"
>       * `valuator->mode = XIModeAbsolute` should have `==`

Gotta love last minute changes...

> 
>   - The "Map Buttons..." button doesn't appear under X11 (but does under
> Wayland)

Filed it as https://bugzilla.gnome.org/show_bug.cgi?id=784882 with a patch.

> 
>   - Clicking "Map Buttons..." for my Cintiq 24HDT causes GNOME to crash

Not sure about that one without a backtrace... I tried to hack my setup into believing I had a 24HDT, and came up with another crash fix, but might not be yours...

> 
>   - Clicking "Map Buttons..." for my Intuos Pro 2 works, but...
>       * The list of options available to the ring includes more than
> keystrokes

Right, addressed that in a separate patch

>       * Mapping the ring to a keystroke (keypad '5') doesn't send the right
> events

Hmm, yeah, I need numlock to have the tablet send '5'. But it's the same with other latched state, if I map a tablet button to 'a' and have capslock enabled on the keyboard, the app gets 'A'. AFAICT this has always been broken, hasn't it?

And it does seem like we get it right in wayland, probably the code brute forcing the right keycode fares slightly better than XTestFakeKeyEvent() with numlock.

Review of attachment 354974 [details] [review]:

Not the most pretty (0/1 doesn't immediately suggest up/down/(counter-)clockwise ...), but fine afaics.

Review of attachment 354976 [details] [review]:

::: clutter/clutter/x11/clutter-device-manager-xi2.c
@@ +379,3 @@
+      /* FIXME: Is there a better way to detect strips? */
+      else if (valuator->label == None &&
+               valuator->mode = XIModeAbsolute && valuator->max > 1)

Accidental assignment? Needs an extra pair of parentheses otherwise ...

@@ +1317,3 @@
+                      }
+
+                    //clutter_event_set_device (event, source_device);

Left-over? If it's something we should do but can't, a comment would be good.

@@ +1349,3 @@
             /* Pad buttons are 0-indexed */
             event->pad_button.button = xev->detail - 1;
+            //clutter_event_set_device (event, source_device);

Dto.

Review of attachment 355464 [details] [review]:

::: src/backends/meta-input-settings.c
@@ +2032,3 @@
+
+      if (accel1 && *accel1 && accel2 && *accel2)
+        str = g_strdup_printf ("%s / %s", accel1 ? accel1 : "", accel2 ? accel2 : "");

Did you mean something like (accel1 && *accel1 || accel2 && *accel2)? Otherwise the strdup can just use accel1 and accel2 without further checks ...

@@ +2037,3 @@
+      g_free (accel2);
+
+      return str;

Mmh, a bit on the late side for an early return ... the alternative would be to split out the buttons/no-buttons specifics into their own functions, but get_pad_weird_non_button_control_action_label() isn't exactly catchy

Review of attachment 355465 [details] [review]:

Commit itself looks obviously fine, just a question on the existing code:

::: js/ui/padOsd.js
@@ +552,3 @@
 
+    _applyLabel: function(label, action, idx, dir, str) {
+        if (str != null) {

Already the case, but: If str == null, we show the label with whatever content it had before - is that intended?

Review of attachment 355466 [details] [review]:

::: js/ui/padOsd.js
@@ +582,3 @@
         let editedLabel;
+
+        if (this._curEdited)

Somewhat unrelated:
Spidermonkey now spills a warning if the property is undefined, so this would be a good time to add
  this._curEdited = null;
  this._prevEdited = null;
to _init()

@@ +832,3 @@
+    },
+
+    _followUpActionEdition: function (str) {

Maybe avoid code duplication with something like:

  let { type, dir, number, mode } = this._editedAction;
  let handler = null;

  if (type == Meta.PadActionType.RING && dir == CCW)
      handler = this._startRingActionEdition;
  else
      handler = this._startStripActionEdition;

  if (handler) {
      this._padDiagram.stopEdition(true, str);
      this._editedAction = null;
      handler(number, (dir + 1) % 2, mode);
      return true;
    }
    return false;

@@ +876,3 @@
 
+        this._endActionEdition();
+        this._setEditedAction(type, number, dir, mode);

Instead of a separate function, just do
  this._editedAction = { type, number, dir, mode };

@@ +893,3 @@
+    _startRingActionEdition: function (ring, dir, mode) {
+        let ch = String.fromCharCode('A'.charCodeAt() + ring);
+        let key = 'ring' + ch + '-' + ((dir == CCW) ? 'ccw' : 'cw') + '-mode-' + mode;

Maybe it's just me, but I find C format strings more readable than string concatenation:
  let key = 'ring%s-%s-mode-%d'.format(ch, dir == CCW ? 'ccw' : 'cw', mode);

Alternatively, js now allows for
  let key = `ring${ch}-${dir == CCW ? 'ccw' : 'cw'}-mode-${mode}`

Review of attachment 355467 [details] [review]:

::: js/ui/padOsd.js
@@ +196,3 @@
+
+    setButtonActionsActive: function (active) {
+        for (let i = 0; i < this._buttonItems.length; i++) {

Or:
this._buttonitems.forEach(item => { item.setSensitive(active); });

Review of attachment 355468 [details] [review]:

Oh my :-(

(In reply to Florian Müllner from comment #13)
> Review of attachment 354974 [details] [review] [review]:
> 
> Not the most pretty (0/1 doesn't immediately suggest
> up/down/(counter-)clockwise ...), but fine afaics.

I agree. Turned those checks into a MetaPadDirection enum with non-mixable up/down and cw/ccw values. Will give another test run before pushing.

(In reply to Florian Müllner from comment #14)
> Review of attachment 354976 [details] [review] [review]:
> 
> ::: clutter/clutter/x11/clutter-device-manager-xi2.c
> @@ +379,3 @@
> +      /* FIXME: Is there a better way to detect strips? */
> +      else if (valuator->label == None &&
> +               valuator->mode = XIModeAbsolute && valuator->max > 1)
> 
> Accidental assignment? Needs an extra pair of parentheses otherwise ...

Meh, left the fixup in the other trou^Wlaptop.

> 
> @@ +1317,3 @@
> +                      }
> +
> +                    //clutter_event_set_device (event, source_device);
> 
> Left-over? If it's something we should do but can't, a comment would be good.

Uh, no, this must be uncommented now that commit 56c468a2 went in. Did locally.

> 
> @@ +1349,3 @@
>              /* Pad buttons are 0-indexed */
>              event->pad_button.button = xev->detail - 1;
> +            //clutter_event_set_device (event, source_device);
> 
> Dto.

This one is with that commit in place :). Went away on rebase.

(In reply to Florian Müllner from comment #15)
> Review of attachment 355464 [details] [review] [review]:
> 
> ::: src/backends/meta-input-settings.c
> @@ +2032,3 @@
> +
> +      if (accel1 && *accel1 && accel2 && *accel2)
> +        str = g_strdup_printf ("%s / %s", accel1 ? accel1 : "", accel2 ?
> accel2 : "");
> 
> Did you mean something like (accel1 && *accel1 || accel2 && *accel2)?
> Otherwise the strdup can just use accel1 and accel2 without further checks
> ...

Oh right, the check for both strings being non-empty is the later change.

> 
> @@ +2037,3 @@
> +      g_free (accel2);
> +
> +      return str;
> 
> Mmh, a bit on the late side for an early return ... the alternative would be
> to split out the buttons/no-buttons specifics into their own functions, but
> get_pad_weird_non_button_control_action_label() isn't exactly catchy

Yeah... I basically failed to find a term that nicely covers both, "knobs" or "sensors" don't really cut it, and "ring_strip" was a bit meh too.

(In reply to Florian Müllner from comment #16)
> Review of attachment 355465 [details] [review] [review]:
> 
> Commit itself looks obviously fine, just a question on the existing code:
> 
> ::: js/ui/padOsd.js
> @@ +552,3 @@
>  
> +    _applyLabel: function(label, action, idx, dir, str) {
> +        if (str != null) {
> 
> Already the case, but: If str == null, we show the label with whatever
> content it had before - is that intended?

Yup, that happens on cancellation, the ActionEditor is hidden and the label is shown as it was before edition. Thinking of it, "restore" is a better verb than "apply".

(In reply to Florian Müllner from comment #17)
> Review of attachment 355466 [details] [review] [review]:
> 
> ::: js/ui/padOsd.js
> @@ +582,3 @@
>          let editedLabel;
> +
> +        if (this._curEdited)
> 
> Somewhat unrelated:
> Spidermonkey now spills a warning if the property is undefined, so this
> would be a good time to add
>   this._curEdited = null;
>   this._prevEdited = null;
> to _init()

Indeed :)

> 
> @@ +832,3 @@
> +    },
> +
> +    _followUpActionEdition: function (str) {
> 
> Maybe avoid code duplication with something like:
> 
>   let { type, dir, number, mode } = this._editedAction;
>   let handler = null;
> 
>   if (type == Meta.PadActionType.RING && dir == CCW)
>       handler = this._startRingActionEdition;
>   else
>       handler = this._startStripActionEdition;
> 
>   if (handler) {
>       this._padDiagram.stopEdition(true, str);
>       this._editedAction = null;
>       handler(number, (dir + 1) % 2, mode);
>       return true;
>     }
>     return false;

Yup, good idea.

> 
> @@ +876,3 @@
>  
> +        this._endActionEdition();
> +        this._setEditedAction(type, number, dir, mode);
> 
> Instead of a separate function, just do
>   this._editedAction = { type, number, dir, mode };

Aye.

> 
> @@ +893,3 @@
> +    _startRingActionEdition: function (ring, dir, mode) {
> +        let ch = String.fromCharCode('A'.charCodeAt() + ring);
> +        let key = 'ring' + ch + '-' + ((dir == CCW) ? 'ccw' : 'cw') +
> '-mode-' + mode;
> 
> Maybe it's just me, but I find C format strings more readable than string
> concatenation:
>   let key = 'ring%s-%s-mode-%d'.format(ch, dir == CCW ? 'ccw' : 'cw', mode);
> 
> Alternatively, js now allows for
>   let key = `ring${ch}-${dir == CCW ? 'ccw' : 'cw'}-mode-${mode}`

For some reason .format() rarely comes to head :), will do that.

(In reply to Florian Müllner from comment #18)
> Review of attachment 355467 [details] [review] [review]:
> 
> ::: js/ui/padOsd.js
> @@ +196,3 @@
> +
> +    setButtonActionsActive: function (active) {
> +        for (let i = 0; i < this._buttonItems.length; i++) {
> 
> Or:
> this._buttonitems.forEach(item => { item.setSensitive(active); });

Yup, that looks better.

(In reply to Florian Müllner from comment #19)
> Review of attachment 355468 [details] [review] [review]:
> 
> Oh my :-(

Kind of a mixed message with the a-c-n :p. Maybe I can pass the func to the _createLabel helper, so the check is performed in a single place for all 3 action types.

(In reply to Carlos Garnacho from comment #20)
> (In reply to Florian Müllner from comment #13)
> > +    _applyLabel: function(label, action, idx, dir, str) {
> > +        if (str != null) {
> > 
> > Already the case, but: If str == null, we show the label with whatever
> > content it had before - is that intended?
> 
> Yup, that happens on cancellation, the ActionEditor is hidden and the label
> is shown as it was before edition. Thinking of it, "restore" is a better
> verb than "apply".

Wait - str can be the newly set keybinding, no?


> (In reply to Florian Müllner from comment #19)
> > Review of attachment 355468 [details] [review] [review] [review]:
> > 
> > Oh my :-(
> 
> Kind of a mixed message with the a-c-n :p.

Nah, it's just a bit sad that we have to do checks like these. "The device has n thingies, but the corresponding magic SVG doesn't have as many slots to label them" ...

(In reply to Florian Müllner from comment #21)
> (In reply to Carlos Garnacho from comment #20)
> > (In reply to Florian Müllner from comment #13)
> > > +    _applyLabel: function(label, action, idx, dir, str) {
> > > +        if (str != null) {
> > > 
> > > Already the case, but: If str == null, we show the label with whatever
> > > content it had before - is that intended?
> > 
> > Yup, that happens on cancellation, the ActionEditor is hidden and the label
> > is shown as it was before edition. Thinking of it, "restore" is a better
> > verb than "apply".
> 
> Wait - str can be the newly set keybinding, no?

Yes. I took this approach because the other possibility is a resync of every label.

> 
> 
> > (In reply to Florian Müllner from comment #19)
> > > Review of attachment 355468 [details] [review] [review] [review] [review]:
> > > 
> > > Oh my :-(
> > 
> > Kind of a mixed message with the a-c-n :p.
> 
> Nah, it's just a bit sad that we have to do checks like these. "The device
> has n thingies, but the corresponding magic SVG doesn't have as many slots
> to label them" ...

FWIW this shouldn't happen regularly, modulo broken detection of features (the x11 ring/strip detection is not the most solid piece of work), or wrong libwacom description/svg.

Still, probably a crashed session is not the best way to tell the user.

(In reply to Carlos Garnacho from comment #22)
> (In reply to Florian Müllner from comment #21)
> > (In reply to Carlos Garnacho from comment #20)
> > > Yup, that happens on cancellation, the ActionEditor is hidden and the label
> > > is shown as it was before edition. Thinking of it, "restore" is a better
> > > verb than "apply".
> > 
> > Wait - str can be the newly set keybinding, no?
> 
> Yes. I took this approach because the other possibility is a resync of every
> label.

Then "apply" sounds right to me ...


> Still, probably a crashed session is not the best way to tell the user.

Oh, I totally agree - when I marked the patch as a-c-n, I really meant it :-)

(It just reminded me of the joy of poking around in an SVG in mysterious ways)

Attachment 355465 [details] pushed as 7991a53 - padOsd: Refactor function setting label after edition into separate function
Attachment 355468 [details] pushed as 0d0e90c - padOsd: strengthen ring/strip label creation

Comment on attachment 354974 [details] [review]
backends: Map tablet pad rings/strips to action settings

Pushed this one after adding some more type safety to directions than 0/1.

Attachment 354974 [details] pushed as ef13ee4 - backends: Map tablet pad rings/strips to action settings

Created attachment 355649 [details] [review]
clutter/x11: Emit CLUTTER_PAD_RING/STRIP events on X11

These events will be useful on gnome-shell UI, so translate the
4-5 button events with exotic axes to those. Also use the
XI_Motion event received when first touching those to reset
the ring/strip state, so we don't receive spurious direction
changes in the upper layers.

Created attachment 355651 [details] [review]
backends: Extend pad action label checks to rings/strips

This way the pad OSD can obtain the keycombos that are mapped to these
for labeling purposes.

Created attachment 355653 [details] [review]
clutter: Add clutter_event_get_pad_event_details()

This function extracts pad event information, and more importantly
exposes it for gobject introspection.

Created attachment 355655 [details] [review]
padOsd: Implement edition of actions for rings/strips

Customization of keycombo actions for strips/rings was lost in the
porting to new incarnation of Wacom support.

The UI here is slightly different, instead of requiring the user to
rotate/swipe in each direction to map each keycombo, the UI will
navigate the user through edition of both options, first one, then
the other.

Also, currently just maps actions on mode=0, mapping actions on
different modes require Clutter API additions that will be added
in future commits.

Created attachment 355656 [details] [review]
padOsd: Use ClutterEvent API to access pad ring/strip number/mode

Review of attachment 355649 [details] [review]:

Looks good as far as I can see (I'll have to trust you on some of the intrinsics)

Review of attachment 355651 [details] [review]:

LGTM

Review of attachment 355653 [details] [review]:

::: clutter/clutter/clutter-event.c
@@ +2186,3 @@
+ * @number: (out): ring/strip/button number
+ * @mode: (out): pad mode as per the event
+ * @value: (out): event axis value

Using annotations of

  (out) (optional):

is more accurate, although I don't know if any bindings actually uses this (gjs certainly doesn't)

@@ +2200,3 @@
+  guint n, m;
+  gdouble v;
+

Throw in a

  g_return_val_if_fail (event != NULL, FALSE);

for good measure?

Review of attachment 355655 [details] [review]:

LGTM

::: js/ui/padOsd.js
@@ +844,3 @@
+            this._startRingActionEdition(number, (dir + 1) % 2, mode);
+        else
+            this._startStripActionEdition(number, (dir + 1) % 2, mode);

I suggested the mod shenanigans for computing the "other" direction generically, but that's no longer necessary with this (much cleaner) code, so just use CW/DOWN ...

Review of attachment 355656 [details] [review]:

Is there a reason for keeping it separate from the previous patch?

Thanks! Agree to the suggested changes, some comments:

(In reply to Florian Müllner from comment #31)
> Review of attachment 355649 [details] [review] [review]:
> 
> Looks good as far as I can see (I'll have to trust you on some of the
> intrinsics)

:). reading xf86-input-wacom code, with small tweaks I think I can remove the FIXME. Still entirely driver-specific, but it's not the only place...

(In reply to Florian Müllner from comment #35)
> Review of attachment 355656 [details] [review] [review]:
> 
> Is there a reason for keeping it separate from the previous patch?

Depends on how much are you willing to admit on gnome-3-24 :). So far the only "interface change" was the possible extra settings on /org/gnome/desktop/peripherals/tablets, there's no api/abi breaks, no new strings... and fixes the regression to a large extent.

But the extra clutter method is an api addition, even if inconspicuous. If you're ok with with that, yay, it'll save me some more embarrassment. Otherwise I'll live with it.

(In reply to Carlos Garnacho from comment #36)
> (In reply to Florian Müllner from comment #35)
> > Review of attachment 355656 [details] [review] [review] [review]:
> > 
> > Is there a reason for keeping it separate from the previous patch?
> 
> Depends on how much are you willing to admit on gnome-3-24 :). So far the
> only "interface change" was the possible extra settings on
> /org/gnome/desktop/peripherals/tablets, there's no api/abi breaks, no new
> strings... and fixes the regression to a large extent.
> 
> But the extra clutter method is an api addition, even if inconspicuous. If
> you're ok with with that, yay, it'll save me some more embarrassment.
> Otherwise I'll live with it.

Oh, I didn't think of the stable branch. The API addition is clearly fine, it's depending on it in gnome-shell that's the potential issue. I'll have to think about it a bit - can you replace the method call with "throw new Error();" to see what happens? If the additional controls simply don't work until mutter is updated, then I don't see a strong reason for not using the new API. If we end up in some inconsistent state that's impossible to get out of, then that's a different story :-)

(In reply to Florian Müllner from comment #37)
> (In reply to Carlos Garnacho from comment #36)
> > (In reply to Florian Müllner from comment #35)
> > > Review of attachment 355656 [details] [review] [review] [review] [review]:
> > > 
> > > Is there a reason for keeping it separate from the previous patch?
> > 
> > Depends on how much are you willing to admit on gnome-3-24 :). So far the
> > only "interface change" was the possible extra settings on
> > /org/gnome/desktop/peripherals/tablets, there's no api/abi breaks, no new
> > strings... and fixes the regression to a large extent.
> > 
> > But the extra clutter method is an api addition, even if inconspicuous. If
> > you're ok with with that, yay, it'll save me some more embarrassment.
> > Otherwise I'll live with it.
> 
> Oh, I didn't think of the stable branch. The API addition is clearly fine,
> it's depending on it in gnome-shell that's the potential issue. I'll have to
> think about it a bit - can you replace the method call with "throw new
> Error();" to see what happens? If the additional controls simply don't work
> until mutter is updated, then I don't see a strong reason for not using the
> new API. If we end up in some inconsistent state that's impossible to get
> out of, then that's a different story :-)

Shouldn't be a big deal. This is actually the entrance point to the whole feature, if we throw an exception in those places, worst that will happen besides the spam in logs is that the feature will be just as inaccessible. You should still be able to map buttons and hit Esc or unplug the tablet to dismiss the dialog.

I will confirm/squash/push tomorrow though, after I reach the desk and testing devices.

(In reply to Carlos Garnacho from comment #38)
> Shouldn't be a big deal. This is actually the entrance point to the whole
> feature, if we throw an exception in those places, worst that will happen
> besides the spam in logs is that the feature will be just as inaccessible.

If that turns out to be the case, then I'm for squashing it. The next stable mutter/gnome-shell release will be in lock-step, so it's reasonable to expect distros to update the two packages at the same time - simply not failing catastrophically when that's not the case is good enough IMHO.

Attachment 355649 [details] pushed as 5c3b27d - clutter/x11: Emit CLUTTER_PAD_RING/STRIP events on X11
Attachment 355651 [details] pushed as f852d2b - backends: Extend pad action label checks to rings/strips
Attachment 355653 [details] pushed as ead6556 - clutter: Add clutter_event_get_pad_event_details()

Testing proved positive, so I merged and pushed :). I see minor usability
warts, but I'll open new bugs for these.

Attachment 355467 [details] pushed as 5202181 - padOsd: Disallow help/switch monitor actions on rings/strips
Attachment 355655 [details] pushed as c64a38f - padOsd: Implement edition of actions for rings/strips

Review of attachment 355649 [details] [review]:

Sorry to barge in uninvited, but Mutter is unbuildable with some options.

::: clutter/clutter/x11/clutter-device-manager-xi2.c
@@ +1154,3 @@
+                ? "pad ring  "
+                : "pad strip",
+                (unsigned int) stage_x11->xwin,

This is broken with --enable-debug.

Thanks for the heads up, fixed in commit c9937faf.