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 <[email protected]>
>> ---
>> 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
>
> [...]
>
>> diff --git a/src/evdev-mt-touchpad-buttons.c
>> b/src/evdev-mt-touchpad-buttons.c
>> index b4d3920..566880f 100644
>> --- a/src/evdev-mt-touchpad-buttons.c
>> +++ b/src/evdev-mt-touchpad-buttons.c
>> @@ -31,6 +31,10 @@
>>
>> #include "evdev-mt-touchpad.h"
>>
>> +#ifndef INPUT_PROP_TOPBUTTONPAD
>> +#define INPUT_PROP_TOPBUTTONPAD 0x04
>> +#endif
>> +
>> #define DEFAULT_BUTTON_MOTION_THRESHOLD 0.02 /* 2% of size */
>> #define DEFAULT_BUTTON_ENTER_TIMEOUT 100 /* ms */
>> #define DEFAULT_BUTTON_LEAVE_TIMEOUT 300 /* ms */
>> @@ -56,6 +60,10 @@ button_state_to_str(enum button_state state) {
>> CASE_RETURN_STRING(BUTTON_STATE_BOTTOM);
>> CASE_RETURN_STRING(BUTTON_STATE_BOTTOM_NEW);
>> CASE_RETURN_STRING(BUTTON_STATE_BOTTOM_TO_AREA);
>> + CASE_RETURN_STRING(BUTTON_STATE_TOP);
>> + CASE_RETURN_STRING(BUTTON_STATE_TOP_NEW);
>> + CASE_RETURN_STRING(BUTTON_STATE_TOP_TO_IGNORE);
>> + CASE_RETURN_STRING(BUTTON_STATE_IGNORE);
>> }
>> return NULL;
>> }
>> @@ -65,6 +73,9 @@ button_event_to_str(enum button_event event) {
>> switch(event) {
>> CASE_RETURN_STRING(BUTTON_EVENT_IN_BOTTOM_R);
>> CASE_RETURN_STRING(BUTTON_EVENT_IN_BOTTOM_L);
>> + CASE_RETURN_STRING(BUTTON_EVENT_IN_TOP_R);
>> + CASE_RETURN_STRING(BUTTON_EVENT_IN_TOP_M);
>> + CASE_RETURN_STRING(BUTTON_EVENT_IN_TOP_L);
>> CASE_RETURN_STRING(BUTTON_EVENT_IN_AREA);
>> CASE_RETURN_STRING(BUTTON_EVENT_UP);
>> CASE_RETURN_STRING(BUTTON_EVENT_PRESS);
>> @@ -94,6 +105,34 @@ is_inside_bottom_left_area(struct tp_dispatch *tp,
>> struct tp_touch *t)
>> !is_inside_bottom_right_area(tp, t);
>> }
>>
>> +static inline bool
>> +is_inside_top_button_area(struct tp_dispatch *tp, struct tp_touch *t)
>> +{
>> + return t->y <= tp->buttons.top_area.bottom_edge;
>> +}
>> +
>> +static inline bool
>> +is_inside_top_right_area(struct tp_dispatch *tp, struct tp_touch *t)
>> +{
>> + return is_inside_top_button_area(tp, t) &&
>> + t->x > tp->buttons.top_area.rightbutton_left_edge;
>> +}
>> +
>> +static inline bool
>> +is_inside_top_left_area(struct tp_dispatch *tp, struct tp_touch *t)
>> +{
>> + return is_inside_top_button_area(tp, t) &&
>> + t->x < tp->buttons.top_area.leftbutton_right_edge;
>> +}
>> +
>> +static inline bool
>> +is_inside_top_middle_area(struct tp_dispatch *tp, struct tp_touch *t)
>> +{
>> + return is_inside_top_button_area(tp, t) &&
>> + t->x >= tp->buttons.top_area.leftbutton_right_edge &&
>> + t->x <= tp->buttons.top_area.rightbutton_left_edge;
>> +}
>> +
>> static void
>> tp_button_set_timer(struct tp_dispatch *tp, uint64_t timeout)
>> {
>> @@ -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.
> 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
> <[email protected]> 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
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
