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

[Jonas Ådahl]

On Wed, Jun 04, 2014 at 10:38:19AM +0200, Hans de Goede wrote:
> 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.

Who would want to disable tapping on a device that doesn't support it?
Anyway, seems its 2-1 on this one so I'll stop now.

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

If that'd happen, its already too late if we already started putting
device info retrieval inside the _config_ prefix, since we wouldn't be
able to be consistent any longer.


Jonas

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