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

[Peter Hutterer]

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.

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

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

Cheers,
   Peter