[PATCH libinput 2/2] evdev-mt-touchpad-buttons: Add support for top softbutton area

Peter Hutterer peter.hutterer at who-t.net
Sun Jun 1 21:04:43 PDT 2014 

On Fri, May 30, 2014 at 04:59:22PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 05/29/2014 08:53 AM, Peter Hutterer wrote:
> > On Wed, May 28, 2014 at 03:19:56PM +0200, Hans de Goede wrote:
> >> Add support for the top softbutton area found on some laptops.
> >>
> >> For details of how this works, see the updated
> >> doc/touchpad-softbutton-state-machine.svg diagram.
> >>
> >> Basically this mirrors the state-machine for the bottom softbutton area, with
> >> one exception, if a finger stays at least inner timeout milliseconds in the
> >> top button area and then moves out of it, it will be ignored rather then
> >> become the pointer. This is done so that people using the top buttons together
> >> with a trackstick and accidentally move their finger out of the upper area
> >> don't get spurious pointer movements from the finger on the trackpad.
> >>
> >> This behavior is indentical to xf86-input-synaptics, which also ignores
> >> movements from touches which start in the top button area.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >>  doc/touchpad-softbutton-state-machine.svg | 602 ++++++++++++++++++------------
> >>  src/evdev-mt-touchpad-buttons.c           | 241 +++++++++++-
> >>  src/evdev-mt-touchpad.h                   |  21 +-
> >>  3 files changed, 615 insertions(+), 249 deletions(-)
> >>
> >> diff --git a/doc/touchpad-softbutton-state-machine.svg b/doc/touchpad-softbutton-state-machine.svg
> >> index 1142343..b7028ef 100644
> >> --- a/doc/touchpad-softbutton-state-machine.svg
> >> +++ b/doc/touchpad-softbutton-state-machine.svg
> > 
> > [...]


> >> @@ -153,6 +192,18 @@ tp_button_set_state(struct tp_dispatch *tp, struct tp_touch *t,
> >>  	case BUTTON_STATE_BOTTOM_TO_AREA:
> >>  		tp_button_set_leave_timer(tp, t);
> >>  		break;
> >> +	case BUTTON_STATE_TOP:
> >> +		break;
> >> +	case BUTTON_STATE_TOP_NEW:
> >> +		t->button.curr = event;
> >> +		tp_button_set_enter_timer(tp, t);
> >> +		break;
> >> +	case BUTTON_STATE_TOP_TO_IGNORE:
> >> +		tp_button_set_leave_timer(tp, t);
> >> +		break;
> >> +	case BUTTON_STATE_IGNORE:
> >> +		t->button.curr = 0;
> > 
> > can you send a follow-up to replace the 0s in this file with an NONE enum
> > value? or squash it into this, either one is fine with me.
> 
> Just tried that, and it does not work, we store a button_event in there
> to distinguish between an in l/m/r button event being the cause of getting
> into the top* / bottom* state. If we add a BUTTON_EVENT_NONE to the
> enum button_event we get a lot of :
> 
> evdev-mt-touchpad-buttons.c: In function 'button_event_to_str':
> evdev-mt-touchpad-buttons.c:73:2: warning: enumeration value 'BUTTON_EVENT_NONE' not handled in switch [-Wswitch]
>   switch(event) {
>   ^
> 
> Warnings, so I believe it is best to leave this as is.

aren't those warnings the point though? in almost all cases you can avoid
them with a case BUTTON_EVENT_NONE: abort() which is the appropriate
handling here anyway. IMO the whole point of enums is that we have a limited
set of values and we can cry bloody murder if we use one outside of those
values.

merged, tested and pushed, with a slight change to the commit subject,
changing from 'evdev-mt-touchpad' to just 'touchpad'.

Cheers,
   Peter

> 
> > also, one thing I noticed is that in the diagram we use "inner/outer"
> > timeout, vs "enter/leave" timer here. This should be adjusted.
> 
> Fixed (I've chosen to adjust the diagram text).
> 
> > 
> >> +		break;
> >>  	}
> >>  }
> >>  
> >> @@ -166,6 +217,11 @@ tp_button_none_handle_event(struct tp_dispatch *tp,
> >>  	case BUTTON_EVENT_IN_BOTTOM_L:
> >>  		tp_button_set_state(tp, t, BUTTON_STATE_BOTTOM_NEW, event);
> >>  		break;
> >> +	case BUTTON_EVENT_IN_TOP_R:
> >> +	case BUTTON_EVENT_IN_TOP_M:
> >> +	case BUTTON_EVENT_IN_TOP_L:
> >> +		tp_button_set_state(tp, t, BUTTON_STATE_TOP_NEW, event);
> >> +		break;
> >>  	case BUTTON_EVENT_IN_AREA:
> >>  		tp_button_set_state(tp, t, BUTTON_STATE_AREA, event);
> >>  		break;
> >> @@ -187,6 +243,9 @@ tp_button_area_handle_event(struct tp_dispatch *tp,
> >>  	switch (event) {
> >>  	case BUTTON_EVENT_IN_BOTTOM_R:
> >>  	case BUTTON_EVENT_IN_BOTTOM_L:
> >> +	case BUTTON_EVENT_IN_TOP_R:
> >> +	case BUTTON_EVENT_IN_TOP_M:
> >> +	case BUTTON_EVENT_IN_TOP_L:
> >>  	case BUTTON_EVENT_IN_AREA:
> >>  		break;
> >>  	case BUTTON_EVENT_UP:
> >> @@ -212,6 +271,9 @@ tp_button_bottom_handle_event(struct tp_dispatch *tp,
> >>  					    event);
> >>  		break;
> >>  	case BUTTON_EVENT_IN_AREA:
> >> +	case BUTTON_EVENT_IN_TOP_R:
> >> +	case BUTTON_EVENT_IN_TOP_M:
> >> +	case BUTTON_EVENT_IN_TOP_L:
> > 
> > nitpick: you have AREA, then TOP_RML, in the two hunks above it's the other
> > way round. I'd prefer this to be the same order everywhere, this especially
> > applies down in tp_button_top*_handle_event.
> 
> Fixed.
> 
> > 
> >>  		tp_button_set_state(tp, t, BUTTON_STATE_BOTTOM_TO_AREA, event);
> >>  		break;
> >>  	case BUTTON_EVENT_UP:
> > 
> > [...]
> > 
> >> @@ -412,6 +613,8 @@ tp_init_buttons(struct tp_dispatch *tp,
> >>  
> >>  	tp->buttons.is_clickpad = libevdev_has_property(device->evdev,
> >>  							INPUT_PROP_BUTTONPAD);
> >> +	tp->buttons.has_topbuttons = libevdev_has_property(device->evdev,
> >> +						     INPUT_PROP_TOPBUTTONPAD);
> > 
> > make sure this is aligned please, even if it goes over the 78.
> 
> Fixed.
> 
> > 
> > 
> >>  	if (libevdev_has_event_code(device->evdev, EV_KEY, BTN_MIDDLE) ||
> >>  	    libevdev_has_event_code(device->evdev, EV_KEY, BTN_RIGHT)) {
> >> @@ -434,8 +637,16 @@ tp_init_buttons(struct tp_dispatch *tp,
> >>  	if (tp->buttons.is_clickpad && !tp->buttons.use_clickfinger) {
> >>  		tp->buttons.bottom_area.top_edge = height * .8 + device->abs.min_y;
> >>  		tp->buttons.bottom_area.rightbutton_left_edge = width/2 + device->abs.min_x;
> >> -		tp->buttons.timer_fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> >>  
> >> +		if (tp->buttons.has_topbuttons) {
> >> +			tp->buttons.top_area.bottom_edge = height * .08 + device->abs.min_y;
> >> +			tp->buttons.top_area.rightbutton_left_edge = width * .58 + device->abs.min_x;
> >> +			tp->buttons.top_area.leftbutton_right_edge = width * .42 + device->abs.min_x;
> >> +		} else {
> >> +			tp->buttons.top_area.bottom_edge = 0;
> >> +		}
> >> +
> >> +		tp->buttons.timer_fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> >>  		if (tp->buttons.timer_fd == -1)
> >>  			return -1;
> >>  
> >> @@ -448,6 +659,7 @@ tp_init_buttons(struct tp_dispatch *tp,
> >>  			return -1;
> >>  	} else {
> >>  		tp->buttons.bottom_area.top_edge = INT_MAX;
> >> +		tp->buttons.top_area.bottom_edge = 0;
> > 
> > INT_MIN maybe? none of the current TOP_BUTTONPAD devices have a negative y
> > axes but better safe than sorry. I remember one bugreport where a touchpad
> > had a range of [-foo, +bar] though I suspect that was a local kernel issue.
> 
> Fixed.
> 
> > tested on the t440s, and Reviewed-by: Peter Hutterer
> > <peter.hutterer at who-t.net> for both, with the changes above.
> 
> Thanks, I've pushed the updated patches here:
> 
> http://cgit.freedesktop.org/~jwrdegoede/libinput/log/
> 
> Let me know if they are ok now, and then I'll push them.
> 
> > And I suspect the patch to add the tests for this just accidentally got
> > stuck in your outbox ;)
> 
> Nah, I'm waiting for you to write a test case for the right bottom button
> area to serve as an example / for me from copy and paste from :)
> 
> Regards,
> 
> Hans
>

More information about the wayland-devel mailing list