[PATCH libinput 2/5] touchpad: hook up click method configuration
Hans de Goede
hdegoede at redhat.com
Mon Jan 5 02:28:32 PST 2015
Hi Peter,
First of all the rest of this series (1/5 [3-5]/5) look good and are:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
I've some remarks on this one though.
On 10-12-14 05:15, Peter Hutterer wrote:
> Allow switching between softbuttons and clickfinger on any mt-capable
> clickpad.
>
> This disables top software buttons on T440s when switching to clickfingers.
> That's left as a future project for those that need this exact behavior...
> The top buttons will work if the touchpad is disabled though.
I do not think that this is a good idea, we've consistently treated the top
area buttons as if they are a separate device, injecting them into the trackpoint,
keeping them enabled when the touchpad is disabled, etc.
And I think that it is quite easy to make this work, your existing changes only
need a few tweaks (outlined below), if you want I can take a shot at this
(after I've gone through my email backlog).
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> src/evdev-mt-touchpad-buttons.c | 163 ++++++++++++++++++++++++++++++++++------
> src/evdev-mt-touchpad.c | 6 +-
> src/evdev-mt-touchpad.h | 15 +++-
> src/libinput.h | 6 ++
> 4 files changed, 160 insertions(+), 30 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
> index 6af3fcf..746d663 100644
> --- a/src/evdev-mt-touchpad-buttons.c
> +++ b/src/evdev-mt-touchpad-buttons.c
> @@ -494,10 +494,9 @@ tp_release_all_buttons(struct tp_dispatch *tp,
> }
> }
>
> -void
> +static void
> tp_init_softbuttons(struct tp_dispatch *tp,
> - struct evdev_device *device,
> - double topbutton_size_mult)
> + struct evdev_device *device)
> {
> int width, height;
> const struct input_absinfo *absinfo_x, *absinfo_y;
> @@ -523,6 +522,26 @@ tp_init_softbuttons(struct tp_dispatch *tp,
> }
>
> tp->buttons.bottom_area.rightbutton_left_edge = width/2 + xoffset;
> +}
> +
> +void
> +tp_init_top_softbuttons(struct tp_dispatch *tp,
> + struct evdev_device *device,
> + double topbutton_size_mult)
> +{
> + int width, height;
> + const struct input_absinfo *absinfo_x, *absinfo_y;
> + int xoffset, yoffset;
> + int yres;
> +
> + absinfo_x = device->abs.absinfo_x;
> + absinfo_y = device->abs.absinfo_y;
> +
> + xoffset = absinfo_x->minimum,
> + yoffset = absinfo_y->minimum;
> + yres = absinfo_y->resolution;
> + width = abs(absinfo_x->maximum - absinfo_x->minimum);
> + height = abs(absinfo_y->maximum - absinfo_y->minimum);
>
> if (tp->buttons.has_topbuttons) {
> /* T440s has the top button line 5mm from the top, event
> @@ -545,6 +564,88 @@ tp_init_softbuttons(struct tp_dispatch *tp,
> }
> }
>
> +static inline uint32_t
> +tp_button_config_click_get_methods(struct libinput_device *device)
> +{
> + struct evdev_device *evdev = (struct evdev_device*)device;
> + struct tp_dispatch *tp = (struct tp_dispatch*)evdev->dispatch;
> + uint32_t methods = LIBINPUT_CONFIG_CLICK_METHOD_NONE;
> +
> + if (tp->buttons.is_clickpad) {
> + methods |= LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS;
> + if (tp->has_mt)
> + methods |= LIBINPUT_CONFIG_CLICK_METHOD_CLICKFINGER;
> + }
> +
> + return methods;
> +}
> +
> +static void
> +tp_switch_click_method(struct tp_dispatch *tp)
> +{
> + if (tp->buttons.click_method == tp->buttons.want_click_method)
> + return;
> +
> + tp->buttons.click_method = tp->buttons.want_click_method;
> +
> + tp_clear_state(tp);
I do not think that this is necessary, and likely even harmful,
the only case where the state may be wrong is when a finger
is in a bottom button area when we switch, I think just keeping the
finger state as is is better then forgetting all about it, requiring
the user to lift it and put it down again.
IOW this is simply a too big hammer, and since you already only
switch when no "buttons" are pressed I do not think this is
necessary.
> +
> + switch (tp->buttons.click_method) {
> + case LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS:
> + tp_init_softbuttons(tp, tp->device);
> + break;
> + case LIBINPUT_CONFIG_CLICK_METHOD_CLICKFINGER:
> + case LIBINPUT_CONFIG_CLICK_METHOD_NONE:
> + tp->buttons.bottom_area.top_edge = INT_MAX;
Since you also add a check in tp_notify_softbutton() for
tp->buttons.click_method != LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS
There is no need for this whole changing of tp->buttons.bottom_area.top_edge
dance, and even with this dance the check in tp_notify_softbutton() is still
necessary as with the bottom_area.top_edge change all clicks will simply
be handled as being in the generic area, and will still be processed by
tp_post_softbutton_buttons(), so this whole switch-case can go, and the
splitting up of tp_init_softbuttons can likewise be removed from this patch.
> + break;
> + }
> +}
> +
> +static enum libinput_config_status
> +tp_button_config_click_set_method(struct libinput_device *device,
> + enum libinput_config_click_method method)
> +{
> + struct evdev_device *evdev = (struct evdev_device*)device;
> + struct tp_dispatch *tp = (struct tp_dispatch*)evdev->dispatch;
> +
> + tp->buttons.want_click_method = method;
> +
> + if (tp->buttons.active == 0)
This check is not consistent with the one in tp_post_button_events()
I think it is best to move the check to tp_switch_click_method() itself,
and use the one from tp_post_button_events() there.
> + tp_switch_click_method(tp);
> +
> + return LIBINPUT_CONFIG_STATUS_SUCCESS;
> +}
> +
> +static enum libinput_config_click_method
> +tp_button_config_click_get_method(struct libinput_device *device)
> +{
> + struct evdev_device *evdev = (struct evdev_device*)device;
> + struct tp_dispatch *tp = (struct tp_dispatch*)evdev->dispatch;
> +
> + /* return the one we want, even if it's not active yet */
> + return tp->buttons.want_click_method;
> +}
> +
> +static enum libinput_config_click_method
> +tp_click_get_default_method(struct tp_dispatch *tp)
> +{
> + if (!tp->buttons.is_clickpad)
> + return LIBINPUT_CONFIG_CLICK_METHOD_NONE;
> + else if (libevdev_get_id_vendor(tp->device->evdev) == VENDOR_ID_APPLE)
> + return LIBINPUT_CONFIG_CLICK_METHOD_CLICKFINGER;
> + else
> + return LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS;
> +}
> +
> +static enum libinput_config_click_method
> +tp_button_config_click_get_default_method(struct libinput_device *device)
> +{
> + struct evdev_device *evdev = (struct evdev_device*)device;
> + struct tp_dispatch *tp = (struct tp_dispatch*)evdev->dispatch;
> +
> + return tp_click_get_default_method(tp);
> +}
> +
> int
> tp_init_buttons(struct tp_dispatch *tp,
> struct evdev_device *device)
> @@ -582,15 +683,17 @@ tp_init_buttons(struct tp_dispatch *tp,
>
> tp->buttons.motion_dist = diagonal * DEFAULT_BUTTON_MOTION_THRESHOLD;
>
> - if (libevdev_get_id_vendor(device->evdev) == VENDOR_ID_APPLE)
> - tp->buttons.use_clickfinger = true;
> + tp->buttons.config_method.get_methods = tp_button_config_click_get_methods;
> + tp->buttons.config_method.set_method = tp_button_config_click_set_method;
> + tp->buttons.config_method.get_method = tp_button_config_click_get_method;
> + tp->buttons.config_method.get_default_method = tp_button_config_click_get_default_method;
> + tp->device->base.config.click_method = &tp->buttons.config_method;
>
> - if (tp->buttons.is_clickpad && !tp->buttons.use_clickfinger) {
> - tp_init_softbuttons(tp, device, 1.0);
> - } else {
> - tp->buttons.bottom_area.top_edge = INT_MAX;
> - tp->buttons.top_area.bottom_edge = INT_MIN;
> - }
> + tp->buttons.want_click_method = tp_click_get_default_method(tp);
> + tp->buttons.click_method = !tp->buttons.want_click_method;
> + tp_switch_click_method(tp);
> +
> + tp_init_top_softbuttons(tp, device, 1.0);
>
> tp_for_each_touch(tp, t) {
> t->button.state = BUTTON_STATE_NONE;
> @@ -617,6 +720,12 @@ tp_post_clickfinger_buttons(struct tp_dispatch *tp, uint64_t time)
> uint32_t current, old, button;
> enum libinput_button_state state;
>
> + if (tp->device->suspended)
> + return 0;
> +
> + if (tp->buttons.click_method != LIBINPUT_CONFIG_CLICK_METHOD_CLICKFINGER)
> + return 0;
> +
> current = tp->buttons.state;
> old = tp->buttons.old_state;
>
> @@ -683,7 +792,7 @@ tp_post_physical_buttons(struct tp_dispatch *tp, uint64_t time)
> return 0;
> }
>
> -static void
> +static int
> tp_notify_softbutton(struct tp_dispatch *tp,
> uint64_t time,
> uint32_t button,
> @@ -702,14 +811,17 @@ tp_notify_softbutton(struct tp_dispatch *tp,
> event.value = (state == LIBINPUT_BUTTON_STATE_PRESSED) ? 1 : 0;
> dispatch->interface->process(dispatch, tp->buttons.trackpoint,
> &event, time);
> - return;
> + return 1;
> }
>
> /* Ignore button events not for the trackpoint while suspended */
> - if (tp->device->suspended)
> - return;
> + if (tp->device->suspended ||
> + tp->buttons.click_method != LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS)
> + return 0;
>
> evdev_pointer_notify_button(tp->device, time, button, state);
> +
> + return 1;
> }
>
> static int
> @@ -783,22 +895,27 @@ tp_post_softbutton_buttons(struct tp_dispatch *tp, uint64_t time)
> tp->buttons.click_pending = false;
>
> if (button)
> - tp_notify_softbutton(tp, time, button, is_top, state);
> + return tp_notify_softbutton(tp, time, button, is_top, state);
>
> - return 1;
> + return 0;
> }
>
> int
> tp_post_button_events(struct tp_dispatch *tp, uint64_t time)
> {
> + int rc = 0;
> +
> if (tp->buttons.is_clickpad) {
> - if (tp->buttons.use_clickfinger)
> - return tp_post_clickfinger_buttons(tp, time);
> - else
> - return tp_post_softbutton_buttons(tp, time);
> - }
> + rc = tp_post_clickfinger_buttons(tp, time);
> + if (rc == 0)
> + rc = tp_post_softbutton_buttons(tp, time);
If we swap things here to first call tp_post_softbutton_buttons() then if
a finger is down in the top buttons, it will be handled by
tp_post_softbutton_buttons(), and if it is down elsewhere then
tp_post_clickfinger_buttons() will get a chance to do its thing, afaict this
is the only change needed to make the top buttons work together with clickfinger
for the rest of the touchpad.
As an added advantage this means that a top softbutton release will still
be handled by tp_post_softbutton_buttons -> tp_notify_softbutton, since it gets
called first. And the bottom button release path is identical to the click finger
release path, so we can (and should) refactor the code to have a shared function
for handling button release on clickpads, basically the else bit of the
if (current) .. else ... found in both tp_post_softbutton_buttons() and
tp_post_clickfinger_buttons(), and with the release path shared there is no need
for the whole waiting for buttons.active == 0 before switching the click method.
Hmm, thinking more about this, scrap the above (sorta), I've an even better solution,
for clickpads we always go through tp_post_softbutton_buttons (which should probably
be renamed), and then in tp_notify_softbutton if not dealing with a top button, on a
LIBINPUT_BUTTON_STATE_PRESSED we check the click_method and if it is clickfinger we
overwrite whatever button the softbutton state machine came up with based on
tp->nfingers_down, this way we keep the code path for clickfingers vs softbuttons
99.9% identical, and we can switch method on the fly without needing to wait for
anything (since the button release path is 100% identical with these changes).
>
> - return tp_post_physical_buttons(tp, time);
> + if (tp->buttons.active == 0 && !tp->buttons.click_pending)
> + tp_switch_click_method(tp);
> + } else
> + rc = tp_post_physical_buttons(tp, time);
> +
> + return rc;
> }
>
> int
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 32d6eac..b672a33 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -700,7 +700,7 @@ tp_destroy(struct evdev_dispatch *dispatch)
> free(tp);
> }
>
> -static void
> +void
> tp_clear_state(struct tp_dispatch *tp)
> {
> uint64_t now = libinput_now(tp->device->base.seat->libinput);
> @@ -739,7 +739,7 @@ tp_suspend(struct tp_dispatch *tp, struct evdev_device *device)
> if (tp->buttons.has_topbuttons) {
> evdev_notify_suspended_device(device);
> /* Enlarge topbutton area while suspended */
> - tp_init_softbuttons(tp, device, 1.5);
> + tp_init_top_softbuttons(tp, device, 1.5);
> } else {
> evdev_device_suspend(device);
> }
> @@ -752,7 +752,7 @@ tp_resume(struct tp_dispatch *tp, struct evdev_device *device)
> /* tap state-machine is offline while suspended, reset state */
> tp_clear_state(tp);
> /* restore original topbutton area size */
> - tp_init_softbuttons(tp, device, 1.0);
> + tp_init_top_softbuttons(tp, device, 1.0);
> evdev_notify_resumed_device(device);
> } else {
> evdev_device_resume(device);
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 1450288..85ad025 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -231,7 +231,11 @@ struct tp_dispatch {
> } top_area;
>
> struct evdev_device *trackpoint;
> - } buttons; /* physical buttons */
> +
> + enum libinput_config_click_method click_method;
> + enum libinput_config_click_method want_click_method;
> + struct libinput_device_config_click_method config_method;
> + } buttons;
>
> struct {
> struct libinput_device_config_scroll_method config_method;
> @@ -293,9 +297,9 @@ int
> tp_init_buttons(struct tp_dispatch *tp, struct evdev_device *device);
>
> void
> -tp_init_softbuttons(struct tp_dispatch *tp,
> - struct evdev_device *device,
> - double topbutton_size_mult);
> +tp_init_top_softbuttons(struct tp_dispatch *tp,
> + struct evdev_device *device,
> + double topbutton_size_mult);
>
> void
> tp_remove_buttons(struct tp_dispatch *tp);
> @@ -309,6 +313,9 @@ void
> tp_release_all_buttons(struct tp_dispatch *tp,
> uint64_t time);
>
> +void
> +tp_clear_state(struct tp_dispatch *tp);
> +
> int
> tp_post_button_events(struct tp_dispatch *tp, uint64_t time);
>
> diff --git a/src/libinput.h b/src/libinput.h
> index 0f0b01f..dd58f83 100644
> --- a/src/libinput.h
> +++ b/src/libinput.h
> @@ -101,6 +101,12 @@ extern "C" {
> * On Apple touchpads, no button areas are provided. Instead, use a
> * two-finger click for a right button click, and a three-finger click for a
> * middle button click.
> + *
> + * Clickfinger configuration can be enabled through the
> + * libinput_device_config_click_set_method() call. If clickfingers are
> + * enabled on a touchpad with top software buttons, both button areas are
> + * disabled. The top software button area is enabled whenever the touchpad
> + * is disabled.
And then this comment can be dropped too.
> */
>
> /**
>
Regards,
Hans
More information about the wayland-devel
mailing list