[PATCH libinput 1/6] evdev: add evdev_pointer_notify_physical_button

[Hans de Goede]

Hi,

For some reason some of the patches have not made it to my mailbox,
so I'm reviewing the entire set here, partially from the web archive.

Patches 1 and 2 looks good and are:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

The state machine code in patch 3 has some issues:

In evdev_middlebutton_ldown_handle_event() you do:

+	case MIDDLEBUTTON_EVENT_R_UP:
+		break;

But in evdev_middlebutton_rdown_handle_event() you do:

+	case MIDDLEBUTTON_EVENT_L_UP:
+		middlebutton_state_error(device, event);
+		break;

This is not consistent. Also you forget to cancel the timer
in a whole bunch of code paths, one example is:

Press left button and release before timeout, now we're
back in idle with the timer running and have:

+evdev_middlebutton_idle_handle_event(struct evdev_device *device,
+				     uint64_t time,
+				     enum evdev_middlebutton_event event)

+	case MIDDLEBUTTON_EVENT_TIMEOUT:
+		middlebutton_state_error(device, event);
+		break;

Triggering. I suggest doing what we've ended up doing in most other
state machines and have a set_state helper which is the only one
to ever touch device->middlebutton.state, that can then always
cancel the timer when called, and (re)set the timer and
device->middlebutton.first_event_time for the states where we want
to start the timer on entering the state.

Also you may want to make the sleep in litest_timeout_middlebutton
slightly larger, at least in all other litest_timeout functions the
sleep is somewhat longer then the timeout in libinput to avoid
surprises / races.

Patches 4 - 6 look good and are:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans


On 15-04-15 05:51, Peter Hutterer wrote:
> No functional changes at this point, this merely splits up any physical
> buttons (i.e. that represent buttons that exist on that device) vs. other
> buttons that are emulated in some way or another.
>
> This is in preparation for the addition of middle button emulation.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>   src/evdev-mt-touchpad-buttons.c |  8 ++++----
>   src/evdev.c                     | 17 +++++++++++++----
>   src/evdev.h                     |  5 +++++
>   3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
> index 18c32fd..56a054c 100644
> --- a/src/evdev-mt-touchpad-buttons.c
> +++ b/src/evdev-mt-touchpad-buttons.c
> @@ -734,10 +734,10 @@ tp_post_physical_buttons(struct tp_dispatch *tp, uint64_t time)
>   				state = LIBINPUT_BUTTON_STATE_RELEASED;
>
>   			b = evdev_to_left_handed(tp->device, button);
> -			evdev_pointer_notify_button(tp->device,
> -						    time,
> -						    b,
> -						    state);
> +			evdev_pointer_notify_physical_button(tp->device,
> +							     time,
> +							     b,
> +							     state);
>   		}
>
>   		button++;
> diff --git a/src/evdev.c b/src/evdev.c
> index 5b4b2b6..6f87484 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -138,6 +138,15 @@ evdev_keyboard_notify_key(struct evdev_device *device,
>   }
>
>   void
> +evdev_pointer_notify_physical_button(struct evdev_device *device,
> +				     uint32_t time,
> +				     int button,
> +				     enum libinput_button_state state)
> +{
> +	evdev_pointer_notify_button(device, time, button, state);
> +}
> +
> +void
>   evdev_pointer_notify_button(struct evdev_device *device,
>   			    uint32_t time,
>   			    int button,
> @@ -430,10 +439,10 @@ evdev_button_scroll_button(struct evdev_device *device,
>   		} else {
>   			/* If the button is released quickly enough emit the
>   			 * button press/release events. */
> -			evdev_pointer_notify_button(device, time,
> +			evdev_pointer_notify_physical_button(device, time,
>   					device->scroll.button,
>   					LIBINPUT_BUTTON_STATE_PRESSED);
> -			evdev_pointer_notify_button(device, time,
> +			evdev_pointer_notify_physical_button(device, time,
>   					device->scroll.button,
>   					LIBINPUT_BUTTON_STATE_RELEASED);
>   		}
> @@ -505,7 +514,7 @@ evdev_process_key(struct evdev_device *device,
>   			evdev_button_scroll_button(device, time, e->value);
>   			break;
>   		}
> -		evdev_pointer_notify_button(
> +		evdev_pointer_notify_physical_button(
>   			device,
>   			time,
>   			evdev_to_left_handed(device, e->code),
> @@ -2208,7 +2217,7 @@ release_pressed_keys(struct evdev_device *device)
>   					LIBINPUT_KEY_STATE_RELEASED);
>   				break;
>   			case EVDEV_KEY_TYPE_BUTTON:
> -				evdev_pointer_notify_button(
> +				evdev_pointer_notify_physical_button(
>   					device,
>   					time,
>   					evdev_to_left_handed(device, code),
> diff --git a/src/evdev.h b/src/evdev.h
> index af09baa..f2ce9bc 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -306,6 +306,11 @@ evdev_pointer_notify_button(struct evdev_device *device,
>   			    uint32_t time,
>   			    int button,
>   			    enum libinput_button_state state);
> +void
> +evdev_pointer_notify_physical_button(struct evdev_device *device,
> +				     uint32_t time,
> +				     int button,
> +				     enum libinput_button_state state);
>
>   void
>   evdev_init_natural_scroll(struct evdev_device *device);
>