GNOME Bugzilla – Bug 779986
Handle pad groups in pad OSD
Last modified: 2017-08-17 01:09:32 UTC
In some special setups, there's separate pad devices that are logically grouped together. A real use case is the Wacom EKR, a remote pad with an USB dongle that can be plugged on high end Cintiq tablets, turning it into an "extension" to the built-in pad. In those situations, we should observe these two pads as related, thus allowing to map buttons in either pad from the OSD. I'm attaching a few patches that just make the OSD "titlebar" clickable, if we're dealing with such a pad with more grouped devices, a small arrow will appear indicating the titlebar pops down a menu, and the several devices will be selectable there. Likewise, one may switch between pads while the osd is shown by just pressing a button in any one, the OSD will update to show the pad that was just interacted. Hotplugging is also taken care of, if pads come and go, the arrow+menu will be enabled/disabled. I know Jimmac felt there's enough wasted space in the OSD to fit the layouts of both pads, however that takes serious thinking and IMHO it's important to support the usecase at all first.
Created attachment 347843 [details] [review] clutter: Add clutter_input_device_is_grouped call/vfunc This will be used to query grouped devices (eg. tablets and pads)
Created attachment 347844 [details] [review] wayland: Use clutter_input_device_is_grouped() for tablet grouping Instead of poking the internal libinput device.
Created attachment 347845 [details] [review] padOsd: peek pads attached to the same tablet
Created attachment 347846 [details] [review] padOsd: shuffle title labels hierarchy Add some boxes in between, so we can add the pad chooser button and the overall result is still centered.
Created attachment 347847 [details] [review] padOsd: Add PadChooser class This is a popdown button that allows choosing between pads in the same group.
Created attachment 347848 [details] [review] padOsd: Allow to switch between pads in the same group Whenever there's more than one pad in the same group (eg. Wacom ExpressKey Remotes), show a popdown menu to allow configuring those extra pads. Devices are hot-pluggable, so the popdown menu will update its state whenever pads are added/removed. Also, allow to quickly change between pads by switching to its OSD by just interacting with them. Always given they are in the same group.
Created attachment 347851 [details] screenshot of OSD configuring EKR, device selection popup open
Review of attachment 347845 [details] [review]: OK
Review of attachment 347846 [details] [review]: ::: js/ui/padOsd.js @@ +563,3 @@ + let boxLayout = new Clutter.BoxLayout({ orientation: Clutter.Orientation.HORIZONTAL, + spacing: 12 }); + this._titleBox = new Clutter.Actor({ layout_manager: boxLayout, I'd prefer St.Widget here (or St.BoxLayout with a style class to apply the spacing instead of the layout manager) @@ +570,3 @@ + boxLayout = new Clutter.BoxLayout({ orientation: Clutter.Orientation.VERTICAL, + spacing: 6 }); + let labelBox = new Clutter.Actor({ layout_manager: boxLayout }); Dto.
Review of attachment 347847 [details] [review]: ::: js/ui/padOsd.js @@ +36,3 @@ + _init: function (device, groupDevices, callback) { + this.actor = new St.Button({ style_class: 'pad-chooser-button' }); + this.actor.set_toggle_mode(true); Alternatively: this.actor = new St.Button({ style_class: 'blah', toggle_mode: true }); (either way is fine, just pointing it out) @@ +47,3 @@ + x_align: Clutter.ActorAlign.CENTER, + y_expand: false, + y_align: Clutter.ActorAlign.CENTER }); StBin doesn't take the expand/align properties of children into account. Instead, there are StBin:x-align/StBin:x-fill properties that can be used to define how the child is placed. (It sucks, but to our defense the St class predates the ClutterActor properties ...) Note that x-fill/y-fill defaults to false and x-align/y-align to St.Align.MIDDLE, so you should get the expected result without setting anything on either parent or child. @@ +76,3 @@ + + this._padChooserMenu.addAction(device.get_device_name(), Lang.bind(this, function() { + this._callback(device); A more standard pattern would be: this._padChooserMenu.addAction(device.get_device_name(), () => { this.emit('pad-picked', device); }); But considering that this is more or less a private helper class to the main PadOsd, passing the callback at construction time is fine too.
Review of attachment 347848 [details] [review]: ::: js/ui/padOsd.js @@ +716,3 @@ + this._titleBox.add_child(this._padChooser.actor); + } + this._padChooser.update(this._groupPads); The group-pads are already passed to the constructor and the pad chooser creates the menu from them. So should this be: if (this._padChooser == null) { ... } else { this._padChooser.update(this._groupPads); } here?
Review of attachment 347843 [details] [review]: Hardly an evdev/xi2 expert, but makes sense to me except for the bit pointed out below ::: clutter/clutter/clutter-input-device.c @@ +2275,3 @@ + g_return_val_if_fail (CLUTTER_IS_INPUT_DEVICE (other_device), FALSE); + + return TRUE; Let me guess: Debug left-over?
Review of attachment 347844 [details] [review]: LGTM
(In reply to Florian Müllner from comment #12) > Review of attachment 347843 [details] [review] [review]: > > Hardly an evdev/xi2 expert, but makes sense to me except for the bit pointed > out below > > ::: clutter/clutter/clutter-input-device.c > @@ +2275,3 @@ > + g_return_val_if_fail (CLUTTER_IS_INPUT_DEVICE (other_device), FALSE); > + > + return TRUE; > > Let me guess: Debug left-over? Doh! it is :), I've got no modern cintiq to run the ekr on, this allowed me make do. (In reply to Florian Müllner from comment #11) > Review of attachment 347848 [details] [review] [review]: > > ::: js/ui/padOsd.js > @@ +716,3 @@ > + this._titleBox.add_child(this._padChooser.actor); > + } > + this._padChooser.update(this._groupPads); > > The group-pads are already passed to the constructor and the pad chooser > creates the menu from them. So should this be: > > if (this._padChooser == null) { > ... > } else { > this._padChooser.update(this._groupPads); > } > > here? Right, changed that. (In reply to Florian Müllner from comment #10) > Review of attachment 347847 [details] [review] [review]: > > ::: js/ui/padOsd.js > @@ +76,3 @@ > + > + this._padChooserMenu.addAction(device.get_device_name(), > Lang.bind(this, function() { > + this._callback(device); > > A more standard pattern would be: > > this._padChooserMenu.addAction(device.get_device_name(), () => { > this.emit('pad-picked', device); > }); > > But considering that this is more or less a private helper class to the main > PadOsd, passing the callback at construction time is fine too. I agree this is better idiom, I changed it that way. Also applied the other suggestions, I'm giving it a test drive and attaching the updated patches.
Created attachment 347862 [details] [review] clutter: Add clutter_input_device_is_grouped call/vfunc This will be used to query grouped devices (eg. tablets and pads)
Created attachment 347864 [details] [review] padOsd: shuffle title labels hierarchy Add some boxes in between, so we can add the pad chooser button and the overall result is still centered.
Created attachment 347865 [details] [review] padOsd: Add PadChooser class This is a popdown button that allows choosing between pads in the same group.
Created attachment 347866 [details] [review] padOsd: Allow to switch between pads in the same group Whenever there's more than one pad in the same group (eg. Wacom ExpressKey Remotes), show a popdown menu to allow configuring those extra pads. Devices are hot-pluggable, so the popdown menu will update its state whenever pads are added/removed. Also, allow to quickly change between pads by switching to its OSD by just interacting with them. Always given they are in the same group.
Review of attachment 347862 [details] [review]: LGTM
Review of attachment 347865 [details] [review]: OK ::: js/ui/padOsd.js @@ +40,3 @@ + y_fill: false, + x_align: St.Align.MIDDLE, + y_align: St.Align.MIDDLE }); As mentioned, those are already the defaults ...
Review of attachment 347866 [details] [review]: Yup
Review of attachment 347864 [details] [review]: Code looks fine, but misses the theme bits that add the spacing. I'm afraid the CSS/submodule stuff sucks, here's how it should be done: 1. cd data/theme/gnome-shell-sass; $EDITOR _common.scss; 2. commit and push the changes 3. cd ../..; make theme/*.css 4. squash theme changes + submodule update with the right commit If you don't have sass installed or this is too cumbersome, I can attach an updated patch after you do 1+2 ...
Created attachment 347870 [details] [review] padOsd: shuffle title labels hierarchy Added the theme stuff.
Attachment 347845 [details] pushed as ada21c9 - padOsd: peek pads attached to the same tablet Attachment 347865 [details] pushed as e057333 - padOsd: Add PadChooser class Attachment 347866 [details] pushed as e039871 - padOsd: Allow to switch between pads in the same group Attachment 347870 [details] pushed as e0c0d92 - padOsd: shuffle title labels hierarchy
Attachment 347844 [details] pushed as 29b240e - wayland: Use clutter_input_device_is_grouped() for tablet grouping Attachment 347862 [details] pushed as e081bb3 - clutter: Add clutter_input_device_is_grouped call/vfunc
*** Bug 750745 has been marked as a duplicate of this bug. ***
Just seeing this today for the first time, this somehow slipped through my filters. I did all of my development work for the EKR without a display tablet (or a regular tablet) and my patch at Bug 750745 treats the EKR as a full device... so I'm just today starting to consider what it means to require the EKR to be used with a display tablet. Jason has a system with two Cintiqs - an older one with a pad and newer one without a pad. We weren't able to see the dropdown for either so we weren't able to fully evaluate this solution. A few initial thoughts: 1. The EKR is available for solo purchase as a separate device. At a minimum I think we need to consider users who might choose to use the EKR with an opaque tablet. 1a. Some users have a setup with a Cintiq and an opaque tablet. If their EKR is "paired" with their Cintiq does this prevent them from using the EKR while the pen is on the opaque tablet? 2. From our end, there is nothing about the EKR which requires tablet. To test if my EKR was working I've plugged it into Windows and used it as the only device on the system. It looks like we are forced to link it to a tablet because of Wayland/libinput?
(CCing Peter, he perhaps wants to add upon/correct my comment) (In reply to Aaron Skomra from comment #27) > Just seeing this today for the first time, this somehow slipped > through my filters. > > I did all of my development work for the EKR without a display tablet > (or a regular tablet) and my patch at Bug 750745 treats the EKR as a > full device... so I'm just today starting to consider what it means to > require the EKR to be used with a display tablet. > > Jason has a system with two Cintiqs - an older one with a pad and > newer one without a pad. We weren't able to see the dropdown for > either so we weren't able to fully evaluate this solution. > > A few initial thoughts: > > 1. The EKR is available for solo purchase as a separate device. At a > minimum I think we need to consider users who might choose to use the > EKR with an opaque tablet. That's a fair point, perhaps there should be a way to override the default grouping other than udev rule mangling. > > 1a. Some users have a setup with a Cintiq and an opaque tablet. If > their EKR is "paired" with their Cintiq does this prevent them from > using the EKR while the pen is on the opaque tablet? No, with caveats. Actions/keycombos would still be triggered on the app with the keyboard focus. But if those actions are related to the brush/tool a stylus is using, it should probably be the one from the tablet it's paired with. This is all up to the app though. > > 2. From our end, there is nothing about the EKR which requires tablet. > To test if my EKR was working I've plugged it into Windows and used > it as the only device on the system. It looks like we are forced to > link it to a tablet because of Wayland/libinput? In general, wayland protocols try to focus on doing a thing as concisely as possible. The tablet protocol is no exception, and a disembodied pad fits oddly there, maybe more conceptually than anything else. It might not take much effort to make them a better fit, but the question is/was whether it's an usecase that should be observed in a *tablet* protocol. I remember there were talks about having distinct "buttonbox" libinput API and wayland protocol, but IIRC the final call was: We don't know enough to cover a generic buttonbox usecase, other usecases than pads won't arrive soon, and tablet pads have semantics we don't know/think make sense on such generic interface (eg. action mapping feedback for the OSD)
(In reply to Carlos Garnacho from comment #28) > That's a fair point, perhaps there should be a way to override the default > grouping other than udev rule mangling. tbh, I don't know how to do this sensibly. This did come up when we worked on device groups, but the effort required to have this done reliably (at least within libinput) is high and this is a rather niche case. The best solution would likely be some external tool that creates that udev rule - even though it would require a restart. > > 2. From our end, there is nothing about the EKR which requires tablet. > > To test if my EKR was working I've plugged it into Windows and used > > it as the only device on the system. It looks like we are forced to > > link it to a tablet because of Wayland/libinput? > > In general, wayland protocols try to focus on doing a thing as concisely as > possible. The tablet protocol is no exception, and a disembodied pad fits > oddly there, maybe more conceptually than anything else. > > It might not take much effort to make them a better fit, but the question > is/was whether it's an usecase that should be observed in a *tablet* > protocol. I remember there were talks about having distinct "buttonbox" > libinput API and wayland protocol, but IIRC the final call was: We don't > know enough to cover a generic buttonbox usecase, other usecases than pads > won't arrive soon, and tablet pads have semantics we don't know/think make > sense on such generic interface (eg. action mapping feedback for the OSD) I vaguely remember an agreement that if the EKR was to be used without a tablet, it should be handled by a separate protocol that exposes it as generic button device. It's not a tablet pad in that case and should not be exposed as such. We haven't had a specific use-case that gave us enough information on what exactly is required and creating a catch-all wayland protocol without a use-case is fraught with danger.