[PATCH libinput 0/9] Improvements for clickpad touchpads

[Peter Hutterer]

Just for the archives, Hans and I talked about this off-list.

On Tue, Apr 01, 2014 at 09:54:34AM +0200, Hans de Goede wrote:
> 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.

I think the BUTTON_STATE_TO_OTHER_BUTTON is a good idea.
For the bottom button area I'm not sure that we really need more buttons
than the current left-right split. Middle-click is possible through
left+right click at the same time - I think that should be enough for the
vast majority of use cases. And not having a middle button area on the
bottom will likely reduce erroneous clicks. 

For the top button area that's different, since those laptops have a defined
middle click area. But we can't just extend them to buttons 3-5.
The main issue here is that the top software buttons do have a slightly
different behaviour than the bottom ones and interact differently with
cursor movement. I think it's worth working that into the state diagram to
see what the effect of it is but as I just haven't done so yet.

I would like to keep the buttons semantically defined, so that in the code
we do know what we are dealing with rather than arbitrary "Button 6". IMO
it's better to support a few well defined use-cases than trying to be
everything to everyone.

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

yeah, that should be added. at the moment I just check for LMR buttons on
the touchpad, which is almost as good as the property (or in the case of the
Cypress touchpads even better ;)

same with INPUT_PROP_DIRECT, we should simply assume anything with that set
is a touchscreen, not a touchpad.

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

the idea Hans brought up here is that we can use finger pinning to lock the
fingers to their current position to avoid miniscule changes when the
touchpad surface moves (during a click). That's a great idea, we just need
to figure out what the effect is on move+click or drag+click behaviours -
there's a chance that the finger may halt movement for a short time and that
may or may not be a problem. I don't know :)
 
> 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.

Yes, that's a bug.

Cheers,
   Peter

> 
> 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>