[PATCH libinput 02/10] Add an interface to enable/disable tapping

[Hans de Goede]

Hi,

On 06/04/2014 10:21 AM, Jonas Ådahl wrote:
> On Wed, Jun 04, 2014 at 11:17:58AM +1000, Peter Hutterer wrote:
>> On Tue, Jun 03, 2014 at 10:41:16PM +0200, Jonas Ådahl wrote:
>>> On Tue, Jun 03, 2014 at 03:34:55PM +1000, Peter Hutterer wrote:
>>>> Provide an interface to enable/disable tapping, with a default mapping of
>>>> 1/2/3 fingers mapping to L/R/M button events, respectively.
>>>>
>>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>>> ---
>>>>  src/libinput-private.h | 13 +++++++++
>>>>  src/libinput.c         | 33 +++++++++++++++++++++++
>>>>  src/libinput.h         | 73 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libinput-private.h b/src/libinput-private.h
>>>> index 61cdc79..020167e 100644
>>>> --- a/src/libinput-private.h
>>>> +++ b/src/libinput-private.h
>>>> @@ -69,12 +69,25 @@ struct libinput_seat {
>>>>  	uint32_t button_count[KEY_CNT];
>>>>  };
>>>>  
>>>> +struct libinput_device_config_tap {
>>>> +	int (*count)(struct libinput_device *device);
>>>> +	enum libinput_config_status (*enable)(struct libinput_device *device,
>>>> +					      int enable);
>>>> +	int (*is_enabled)(struct libinput_device *device);
>>>> +	void (*reset)(struct libinput_device *device);
>>>> +};
>>>> +
>>>> +struct libinput_device_config {
>>>> +	struct libinput_device_config_tap *tap;
>>>> +};
>>>> +
>>>>  struct libinput_device {
>>>>  	struct libinput_seat *seat;
>>>>  	struct list link;
>>>>  	void *user_data;
>>>>  	int terminated;
>>>>  	int refcount;
>>>> +	struct libinput_device_config config;
>>>>  };
>>>>  
>>>>  typedef void (*libinput_source_dispatch_t)(void *data);
>>>> diff --git a/src/libinput.c b/src/libinput.c
>>>> index 6b7e8b8..6a713bb 100644
>>>> --- a/src/libinput.c
>>>> +++ b/src/libinput.c
>>>> @@ -1182,3 +1182,36 @@ libinput_event_touch_get_base_event(struct libinput_event_touch *event)
>>>>  {
>>>>  	return &event->base;
>>>>  }
>>>> +
>>>> +LIBINPUT_EXPORT int
>>>> +libinput_device_config_tap_get_finger_count(struct libinput_device *device)
>>>> +{
>>>> +	return device->config.tap ? device->config.tap->count(device) : 0;
>>>> +}
>>>> +
>>>> +LIBINPUT_EXPORT enum libinput_config_status
>>>> +libinput_device_config_tap_enable(struct libinput_device *device,
>>>> +				  int enable)
>>>> +{
>>>> +	if (enable &&
>>>> +	    libinput_device_config_tap_get_finger_count(device) == 0)
>>>> +		return LIBINPUT_CONFIG_STATUS_UNSUPPORTED;
>>>
>>> Shouldn't this be returned also if !enable? It would be a bit confusing
>>> if disabling is allowed, but enabling is not.
>>
>> This was a conscious decision that applies to the other functions as well:
>> if you're doing something that's not technically invalid, we're not
>> reporting an error. This means that an error only needs to be handled if
>> something really is off.
> 
> I'd argue that disabling tapping on a device that doesn't support
> tapping is a technically invalid operation that is simply handled in the
> cases where it makes no sense.

I'm with Peter here, that its easier for our API consumers if they caan
always safely call disable.

> 
>>
>>> Also, in what circumstances will config.tap be set, but finger count
>>> returning 0? What type of devices would have tapping configuration
>>> enabled with no tapping possible?
>>
>> I expect none, this approach was just the safest and most copy-paste-proof
>> way of checking.
>>  
>>>> +
>>>> +	return device->config.tap->enable(device, enable);
>>>> +}
>>>> +
>>>> +LIBINPUT_EXPORT int
>>>> +libinput_device_config_tap_is_enabled(struct libinput_device *device)
>>>> +{
>>>> +	if (libinput_device_config_tap_get_finger_count(device) == 0)
>>>> +		return 0;
>>>> +
>>>> +	return device->config.tap->is_enabled(device);
>>>> +}
>>>> +
>>>> +LIBINPUT_EXPORT void
>>>> +libinput_device_config_tap_reset(struct libinput_device *device)
>>>> +{
>>>> +	if (device->config.tap)
>>>> +		device->config.tap->reset(device);
>>>> +}
>>>> diff --git a/src/libinput.h b/src/libinput.h
>>>> index c9ec71a..0c84547 100644
>>>> --- a/src/libinput.h
>>>> +++ b/src/libinput.h
>>>> @@ -1362,8 +1362,79 @@ enum libinput_config_status {
>>>>  const char *
>>>>  libinput_config_status_to_str(enum libinput_config_status status);
>>>>  
>>>> +/**
>>>> + * @ingroup config
>>>> + *
>>>> + * Check if the device supports tap-to-click. See
>>>> + * libinput_device_config_tap_set() for more information.
>>>
>>> Should it be libinput_device_config_tap_enable() here instead?
>>
>> ah, thanks. one always escapes the refacturing...
>>
>>>> + *
>>>> + * @param device The device to configure
>>>> + * @return The number of fingers that can generate a tap event, or 0 if the
>>>> + * device does not support tapping.
>>>> + *
>>>> + * @see libinput_device_config_tap_enable
>>>> + * @see libinput_device_config_tap_is_enabled
>>>> + * @see libinput_device_config_tap_reset
>>>> + */
>>>> +int
>>>> +libinput_device_config_tap_get_finger_count(struct libinput_device *device);
>>>
>>> I wonder where doing things like this, and
>>> libinput_device_config_scroll_get_methods() will lead us. It'd mean we
>>> expose some device information via the config API, and some via direct
>>> device getters, which might not be the best thing for consistency.
>>
>> Just to be clear - you're talking about ditching the "_config_" part of the
>> interface? If so, yeah, doable but I don't want to ditch the feature prefix.
> 
> Yes, feature prefix I think is better as well, was just talking about
> putting device info retrieval under the _config_ prefix.
> 
>>
>> The main idea behind this approach and hence the _config_ prefix: I tried to
>> encapsulate the config APIs to the extent that it doesn't matter what a
>> device is or how it works. All that matters is "does it support tapping?",
>> and then the decision is on the caller to enable/disable it. That's
>> independent of anything else on the device.
> 
> Yea, I see what you mean. My concern is that there might be cases where
> config alternatives/information overlaps with other device information
> not directly related to device configuration, and it would be slightly
> awkward to go use a config API for such cases. The truth is I have no
> examples of this in my mind right now, so I'm not sure its a valid
> concern.

I would like to suggest to deal with this when we've an actual example
of it. Until then I think having the _config_ in the name is useful.

> 
>>  
>>> Would it be better to keep device info getters, even though they are only
>>> directly relevant to the configuration API, outside of the coniguration
>>> API, so that it'll be the same for all device specific information
>>> retrieval? There could potentially also be some device information that
>>> are relevant to multiple config options, and in those cases, where would
>>> they be retrieved from?
>>
>> the feature-specific set of functions. For example some touchpads provide 2
>> real MT touchpoints but also BTN_TOOL_TRIPLETAP, or no MT touchpoints but
>> BTN_TOOL_DOUBLETAP. In both cases we could support tapping but not
>> necessarily any other 3/2 finger gesture. Exposing this as a single
>> libinput_device_get_finger_count() means we leave guessing what is possible
>> up to the caller.
> 
> Yea, grouping by feature seems sensible to me.

+1

Regards,

Hans