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.
