[PATCH libinput 2/5] touchpad: hook up click method configuration

[Peter Hutterer]

On Mon, Jan 05, 2015 at 11:28:32AM +0100, Hans de Goede wrote:
> 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).

yeah, that'd be great, thanks.  I'll leave this series with you then, it's
on github in the wip/clickfinger-configuration branch, rebased on master.
only changes were moving the documentation to the new location and adding
the new calls to libinput.sym.

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

ok, works for me.

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

the main reason I dropped the top edge was to limit the cognitive load on
the button states. With the edge of INT_MAX we know that the state is always
AREA or NONE, so it shouldn't ever interfere with anything.

it may be worth doing this more expressively, i.e. adding an explicit check
to tp_button_handle_state() for the click method and skip the button
handling. but yeah, with your suggestion below that won't work if we still
track the top buttons. so feel free to drop this and implement the method
you outlined below (always going through tp_post_softbutton_buttons). that
sounds like the best and simplest plan.

Cheers,
   Peter
 
> >+		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.

yep, works for me

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