[PATCH libinput 0/5] Conditional touchpad disabling

[Peter Hutterer]

On Sun, Sep 14, 2014 at 08:39:17PM +0200, Hans de Goede wrote:
> Hi Peter, et al,
> 
> On 09/04/2014 08:31 AM, Peter Hutterer wrote:
> > 
> > This patchset adds support for two features:
> > * disabling the touchpad when a mouse is plugged in
> > * allow top-software buttons to work when the touchpad is disabled
> > 
> > Builds on the patchset sent out here:
> > http://lists.freedesktop.org/archives/wayland-devel/2014-September/017032.html
> 
> Patches 1-2 look good to me, they are:
> 
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> 
> For patch 3, esp this bit:
> 
> +static void
> +tp_device_added(struct evdev_device *device,
> +		struct evdev_device *added_device)
> +{
> +	struct tp_dispatch *tp = (struct tp_dispatch*)device->dispatch;
> +
> +	if (tp->sendevents.current_mode !=
> +	    LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_EXTERNAL_MOUSE)
> +		return;
> +
> +	if (added_device->tags & EVDEV_TAG_EXTERNAL_MOUSE)
> +		tp_suspend(tp, device);
> +}
> 
> Combined with Bastien's comments, has made me think that we need to treat
> tp->sendevents.current_mode as a bitmask rather then an enum. I see that
> the values for the libinput_config_send_events_mode enum already are bitmask
> values, rather then incremental integer values.
> 
> So what I'm thinking is e.g. adding a
> 	LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_TRACKPOINT = (1 << 3)
> to the enum, make sure that the bitmasks for various tags match their
> LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_foo counterparts, and then we can
> change the above into:

> 
> +static void
> +tp_device_added(struct evdev_device *device,
> +		struct evdev_device *added_device)
> +{
> +	struct tp_dispatch *tp = (struct tp_dispatch*)device->dispatch;
> +
> +	if (added_device->tags & tp->sendevents.current_mode)
> +		tp_suspend(tp, device);
> +}
> 
> And then libinput users can do things like:
> 
> libinput_device_config_send_events_set_mode(dev,
> 	LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_EXTERNAL_MOUSE |
> 	LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_TRACKPOINT)
> 
> And we can express all the weird combinations Bastien has indicated he
> may want, without needing any additional complexity inside libinput.
> 
> Note if we do this we should probably rename sendevents.current_mode,
> and we need to sanity check what gets passed into
> libinput_device_config_send_events_set_mode(0 a bit harder so that
> an unconditional enable / disable cannot be mixed with one of the
> LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_foo options.

so generally the bitmask makes sense but we do have to worry about
combinatorial explosion in the test matrix. And I wonder about some of the
cases (I'll reply to the email separately in that thread).

I wonder if the HW_DISABLED case is special enough to warrant _not_ putting
it into the API but instead only providing the backchannel configuration via
udev. If a device is broken enough to be disabled altogether this seems like
a static permanent configuration rather than one that the compositor would
apply every time the device comes up.

> Patch 4 is:
> 
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> 
> Patch 5 I'm not all that enthusiastic about, I see 2 issues with it:
> 
> 1) Only rerouting the top buttons to the trackpoint when the touchpad
> is disabled means the origin of the buttons becomes inconsistent
> over time. I would like to suggest to always send the top buttons
> events through the trackpoint if there is a trackpoint, this is something
> which we will need anyways for middle button scrolling on the trackpoint.

Yeah, fair enough. Better have that stable over time, though IIRC Benjamin
said he's one user of the top buttons without using the trackstick itself so
really we can't win anyway.  but I think it's reasonable to say that the
manufacturer intended for those to be related to the trackstick.

> 2) Extending the top button area to the entire pad when disabled is
> a clever hack. A bit to clever IMHO, this means that the behavior of
> e.g. clicking in the center of the pad changes from registering a
> left click to registering a middle click when the pad gets "disabled"
> again this is inconsistent, and breaches the principle of least
> surprise. A user can likely understand clicking outside of the top button
> area not working when the touchpad is disabled. OTOH changing which button
> we send when the touchpad gets disabled will leave the user wondering what
> is going on.

Two points here:
* extending the top button area to be a bit bigger makes sense IMO if we
  know the touchpad is used as button area only (maybe up to 20% or so)
* the top software button behaviour is that a touchpad has to start inside
  the button to be recognised as button. when the touchpad stops being a
  touchpad that behaviour should arguably be changed to alow for moving into
  the right button area and triggering a right click.
  extending the top button area a bit should make that need obsolete though.


> I'm willing to write (a) patch(es) for this, since I need 1) anyways. Please
> let me know if you plan to work on this, or want me to pick it up tomorrow
> morning (CET).

Yeah, happy for you to take this over.

Thanks for the reviews

Cheers,
   Peter