[PATCH libinput 0/9] Improvements for clickpad touchpads

[Hans de Goede]

Hi Peter,

So as you requested I've been looking at your wip/clickpad-improvements
branch. I've been mostly reading the code / patches and here is a long list
of comments :

1) I like the state-machine concept, and having a diagram. I think this is
a good way to deal with this, otherwise we will end up adding layers of
kludges until we've something which no one understands and which works
most of the time

2) You only handle 2 buttons areas left and right, this won't work
for the Thinkpad *40's series. I've been thinking about a way to make
your state-machine work with more buttons without exploding the diagram /
amount of states. I think that we need to define BUTTON_EVENT_IN_BUTTON0 ..
BUTTON_EVENT_IN_BUTTON7, store one of these in t->button.current_button
and then have states like BUTTON_STATE_NEW and events / transition
conditions like "finger in other button area".

Then in the switch cases in the statemachine where we now check for the
other button, check for all buttons, and put an if there to turn the
current button into a break, ie in tp_button_left_handle_event have:

switch(event) {
case BUTTON_EVENT_IN_BUTTON0:
case BUTTON_EVENT_IN_BUTTON1:
case BUTTON_EVENT_IN_BUTTON2:
case BUTTON_EVENT_IN_BUTTON3:
case BUTTON_EVENT_IN_BUTTON4:
case BUTTON_EVENT_IN_BUTTON5:
case BUTTON_EVENT_IN_BUTTON6:
case BUTTON_EVENT_IN_BUTTON7:
	if (event == t->button.current_button)
		break;
	t->button.state = BUTTON_STATE_TO_OTHER_BUTTON;
	tp_button_set_leave_timer(tp, t);
	break;

This will significantly simplify the state-machine while allowing more
buttons, ie BUTTON_STATE_LEFT_TO_RIGHT and BUTTON_STATE_RIGHT_TO_LEFT would
become a single BUTTON_STATE_TO_OTHER_BUTTON state, and likewise
BUTTON_STATE_LEFT_NEW and BUTTON_STATE_RIGHT_NEW would both simply become
BUTTON_STATE_NEW, etc.

I can rework the state machine + code to work like this if you agree that
this is a good idea.

3) You don't seem to do anything with INPUT_PROP_BUTTONPAD, I'm not sure that
is a good idea

4) In pin finger you assume that the finger which is the closest to the
bottom edge is the one doing the clicking, this will not work on the
Thinkpad *40 series. IE there may be a palm on the touchpad which will be
lower then the finger clicking on the trackpoint button areas. I know we
don't have palm detection yet, but in general this just feels wrong. Maybe
just pin all fingers on a click and rely on unpinning to do its job to
allow click + drag ?

5) In tp_post_softbutton_buttons you don't seem to deal with a physical
click coming before, or at the same time, as the finger down this means that
a user ie using the trackpoint and then doing a quick right click, will get
a left click (t->button.state == BUTTON_STATE_RIGHT_NEW) or no click at all
(tp->nfingers_down == 0).  Note that neither will get corrected later since
current == old will be true.

To fix this I suggest that we check if all touch button states are stable,
and if not set a click pending flag and stop there. Then when ever a touch
button state becomes stable check the click pending flag, and if a click
is pending process it then. Note that I say all touch button states must be
stable because one of the things I expect libinput to fix is moving the
pointer with my right input finger and then clicking the right button area
with my right thumb. The thumb will then be the last finger down and we want
it to be stable too so that we properly report a right click.  Note that
this scenario also advocates in favor of pinning all fingers, so that any
movement of the index finger detected by the tp at this time also does not
get reported as this likely is an unwanted side-effect of the click too.

Regards,

Hans