[PATCH libinput 0/5] Conditional touchpad disabling

[Hans de Goede]

Hi,

On 09/15/2014 07:33 AM, Peter Hutterer wrote:
> 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.

Yeah I was worried a bit about that too, but we can simply limit testing
to the most common ones. If we implement this the way I've suggested, and
double / tripple check that the masks match then testing only one bit would
be enough, as all combinations use the same code path.

> And I wonder about some of the
> cases (I'll reply to the email separately in that thread).

I wonder about a lot of the cases. But since with the mask solution as soon
as we add a LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_foo flavor we can support
all combinations with that flavor, I believe it is best to leave the worrying
to the UI designers and let them decide which combinations to expose. Then we
can use the result of that to decide which combinations to test in our test
cases.

> 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.

I think that being able to explicitly disable devices is useful functionality
to have.

> 
>> 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.

Ack.

>> 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.

Ack to both.

>> 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.

Ok, I'll start working on this today.

Regards,

Hans