[PATCH libinput 02/10] Add an interface to enable/disable tapping
Jonas Ådahl
jadahl at gmail.com
Wed Jun 4 01:21:17 PDT 2014
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.
>
> > 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.
>
> > 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.
Jonas
>
> Cheers,
> Peter
More information about the wayland-devel
mailing list