[PATCH v3 weston] clients: Add API for pointer locking and pointer confinement
Jonas Ådahl
jadahl at gmail.com
Sun Nov 1 22:53:02 PST 2015
On Thu, Sep 17, 2015 at 04:28:32PM -0700, Bryce Harrington wrote:
> On Fri, Jun 26, 2015 at 12:37:54PM +0800, Jonas Ådahl wrote:
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >
> > Changes since v2:
> >
> > * Automatically update the lock/confine region when window is resized.
> >
> >
> > Makefile.am | 6 +-
> > clients/window.c | 316 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > clients/window.h | 62 +++++++++++
> > 3 files changed, 383 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 201b780..9ddbbe9 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -497,7 +497,11 @@ nodist_libtoytoolkit_la_SOURCES = \
> > protocol/xdg-shell-protocol.c \
> > protocol/xdg-shell-client-protocol.h \
> > protocol/ivi-application-protocol.c \
> > - protocol/ivi-application-client-protocol.h
> > + protocol/ivi-application-client-protocol.h \
> > + protocol/pointer-lock-protocol.c \
> > + protocol/pointer-lock-client-protocol.h \
> > + protocol/relative-pointer-protocol.c \
> > + protocol/relative-pointer-client-protocol.h
> >
> > BUILT_SOURCES += $(nodist_libtoytoolkit_la_SOURCES)
> >
> > diff --git a/clients/window.c b/clients/window.c
> > index 81e007b..a468ad0 100644
> > --- a/clients/window.c
> > +++ b/clients/window.c
> > @@ -23,6 +23,7 @@
> >
> > #include "config.h"
> >
> > +#include <stdbool.h>
> > #include <stdint.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > @@ -69,7 +70,10 @@ typedef void *EGLContext;
> > #include "xdg-shell-client-protocol.h"
> > #include "text-cursor-position-client-protocol.h"
> > #include "workspaces-client-protocol.h"
> > +#include "pointer-lock-client-protocol.h"
> > +#include "relative-pointer-client-protocol.h"
> > #include "../shared/os-compatibility.h"
> > +#include "../shared/util.h"
> >
> > #include "window.h"
> >
> > @@ -97,6 +101,8 @@ struct display {
> > struct workspace_manager *workspace_manager;
> > struct xdg_shell *xdg_shell;
> > struct ivi_application *ivi_application; /* ivi style shell */
> > + struct _wl_relative_pointer_manager *relative_pointer_manager;
> > + struct _wl_pointer_lock *pointer_lock;
> > EGLDisplay dpy;
> > EGLConfig argb_config;
> > EGLContext argb_ctx;
> > @@ -249,6 +255,8 @@ struct window {
> > window_output_handler_t output_handler;
> > window_state_changed_handler_t state_changed_handler;
> >
> > + window_locked_pointer_motion_handler_t locked_pointer_motion_handler;
> > +
> > struct surface *main_surface;
> > struct xdg_surface *xdg_surface;
> > struct xdg_popup *xdg_popup;
> > @@ -263,6 +271,19 @@ struct window {
> > /* struct surface::link, contains also main_surface */
> > struct wl_list subsurface_list;
> >
> > + struct _wl_relative_pointer *relative_pointer;
> > + struct _wl_locked_pointer *locked_pointer;
> > + struct input *locked_input;
> > + bool pointer_locked;
> > + locked_pointer_locked_handler_t pointer_locked_handler;
> > + locked_pointer_unlocked_handler_t pointer_unlocked_handler;
> > + confined_pointer_confined_handler_t pointer_confined_handler;
> > + confined_pointer_unconfined_handler_t pointer_unconfined_handler;
> > +
> > + struct _wl_confined_pointer *confined_pointer;
> > + struct widget *confined_widget;
> > + bool confined;
> > +
> > void *user_data;
> > struct wl_list link;
> > };
> > @@ -3872,6 +3893,22 @@ window_do_resize(struct window *window)
> >
> > if (!window->fullscreen && !window->maximized)
> > window->saved_allocation = window->pending_allocation;
> > +
> > + if (window->confined && window->confined_widget) {
> > + struct wl_compositor *compositor = window->display->compositor;
> > + struct wl_region *region;
> > + struct widget *widget = window->confined_widget;
> > +
> > + region = wl_compositor_create_region(compositor);
> > + wl_region_add(region,
> > + widget->allocation.x,
> > + widget->allocation.y,
> > + widget->allocation.width,
> > + widget->allocation.height);
> > + _wl_confined_pointer_set_region(window->confined_pointer,
> > + region);
> > + wl_region_destroy(region);
> > + }
> > }
> >
> > static void
> > @@ -4403,6 +4440,43 @@ window_set_state_changed_handler(struct window *window,
> > }
> >
> > void
> > +window_set_pointer_locked_handler(struct window *window,
> > + locked_pointer_locked_handler_t locked)
> > +{
> > + window->pointer_locked_handler = locked;
> > +}
> > +
> > +void
> > +window_set_pointer_unlocked_handler(struct window *window,
> > + locked_pointer_unlocked_handler_t unlocked)
> > +{
> > + window->pointer_unlocked_handler = unlocked;
> > +}
> > +
> > +void
> > +window_set_pointer_confined_handler(
> > + struct window *window, confined_pointer_confined_handler_t confined)
> > +{
> > + window->pointer_confined_handler = confined;
> > +}
> > +
> > +void
> > +window_set_pointer_unconfined_handler(
> > + struct window *window,
> > + confined_pointer_unconfined_handler_t unconfined)
> > +{
> > + window->pointer_unconfined_handler = unconfined;
> > +}
> > +
> > +void
> > +window_set_locked_pointer_motion_handler(
> > + struct window *window,
> > + window_locked_pointer_motion_handler_t handler)
> > +{
> > + window->locked_pointer_motion_handler = handler;
> > +}
> > +
> > +void
> > window_set_title(struct window *window, const char *title)
> > {
> > free(window->title);
> > @@ -4444,6 +4518,239 @@ window_damage(struct window *window, int32_t x, int32_t y,
> > }
> >
> > static void
> > +relative_pointer_handle_motion(void *data, struct _wl_relative_pointer *pointer,
> > + uint32_t time,
> > + int32_t dx_int, int32_t dx_frac,
> > + int32_t dy_int, int32_t dy_frac,
> > + int32_t dx_unaccel_int, int32_t dx_unaccel_frac,
> > + int32_t dy_unaccel_int, int32_t dy_unaccel_frac)
> > +{
> > + struct input *input = data;
> > + struct window *window = input->pointer_focus;
> > +
> > + if (window->locked_pointer_motion_handler &&
> > + window->pointer_locked) {
> > + window->locked_pointer_motion_handler(
> > + window, input, time,
> > + wl_double_fixed_to_double(dx_int, dx_frac),
> > + wl_double_fixed_to_double(dy_int, dy_frac),
> > + window->user_data);
> > + }
> > +}
>
> Totally unimportant cosmetics, but brackets are unnecessary here, and
> some subsequent single-statement conditionals.
AFAIK, the common case is to add brackets simply if the statement(s) are
more than one line, not regarding the number of statements.
>
> > +static const struct _wl_relative_pointer_listener relative_pointer_listener = {
> > + relative_pointer_handle_motion,
> > +};
> > +
> > +static void
> > +locked_pointer_locked(void *data,
> > + struct _wl_locked_pointer *locked_pointer)
> > +{
> > + struct input *input = data;
> > + struct window *window = input->pointer_focus;
> > +
> > + window->pointer_locked = true;
> > +
> > + if (window->pointer_locked_handler) {
> > + window->pointer_locked_handler(window,
> > + input,
> > + window->user_data);
> > + }
> > +}
> > +
> > +static void
> > +locked_pointer_unlocked(void *data,
> > + struct _wl_locked_pointer *locked_pointer)
> > +{
> > + struct input *input = data;
> > + struct window *window = input->pointer_focus;
> > +
> > + window_unlock_pointer(window);
>
> This seems curious that we unlock via a function call here, but set the
> pointer_locked member of window directly in the prior routine. Should
> the previous routine be calling a window_lock_pointer() routine to make
> these two calls implementationally consistent?
"window_unlock_pointer" unlocks the pointer, while the above function is
just a notification that the compositor has locked the cursor. The
action of locking the client does has already been done prior to the
scenario the function above is called, so it'd be misleading to call
anything there "lock_pointer" like.
On the other hand, the function (window_unlock_pointer()) could called
"destroy_lock" or something with a stupid helper called
"window_unlock_pointer()" that simply just calls that. At the time I
didn't think it was worth the extra helper, but if you'd like I can add
it.
>
> > + if (window->pointer_unlocked_handler) {
> > + window->pointer_unlocked_handler(window,
> > + input,
> > + window->user_data);
> > + }
> > +}
> > +
> > +static const struct _wl_locked_pointer_listener locked_pointer_listener = {
> > + locked_pointer_locked,
> > + locked_pointer_unlocked,
> > +};
> > +
> > +int
> > +window_lock_pointer(struct window *window, struct input *input)
> > +{
>
> Is there a specific need for this to return int rather than bool, since
> it appears the return value is being used just for success/failure
> indication?
As far as I can see, error reporting is done by returning -1 and success
by returning 0, everywhere else in toytoolkit.
>
> > + struct _wl_relative_pointer_manager *relative_pointer_manager =
> > + window->display->relative_pointer_manager;
> > + struct _wl_pointer_lock *pointer_lock = window->display->pointer_lock;
> > + struct _wl_relative_pointer *relative_pointer;
> > + struct _wl_locked_pointer *locked_pointer;
> > +
> > + if (!window->display->relative_pointer_manager)
> > + return -1;
> > +
> > + if (!window->display->pointer_lock)
> > + return -1;
> > +
> > + if (window->locked_pointer)
> > + return -1;
> > +
> > + if (window->confined_pointer)
> > + return -1;
> > +
> > + if (!input->pointer)
> > + return -1;
> > +
> > + relative_pointer = _wl_relative_pointer_manager_get_relative_pointer(
> > + relative_pointer_manager, input->pointer);
> > + _wl_relative_pointer_add_listener(relative_pointer,
> > + &relative_pointer_listener,
> > + input);
> > +
> > + locked_pointer =
> > + _wl_pointer_lock_lock_pointer(pointer_lock,
> > + window->main_surface->surface,
> > + input->seat,
> > + NULL);
> > + _wl_locked_pointer_add_listener(locked_pointer,
> > + &locked_pointer_listener,
> > + input);
>
> Is there any chance for _wl_pointer_lock_lock_pointer or
> _wl_relative_pointer_manager_get_relative_pointer to return NULL? If
> so, might want to insert pointer checks (or asserts if NULLs would be
> programmer errors) before passing them to the subsequent wl calls.
Sure, but thats usally not done in toytoolkit. I suppose it could be a
follow up at a later point to clean up OOM checking, but it'd just
introduce inconsistencies doing it in this patch.
>
> > + window->locked_input = input;
> > + window->locked_pointer = locked_pointer;
> > + window->relative_pointer = relative_pointer;
> > +
> > + return 0;
> > +}
> > +
> > +void
> > +window_unlock_pointer(struct window *window)
> > +{
> > + if (!window->locked_pointer)
> > + return;
> > +
> > + _wl_locked_pointer_destroy(window->locked_pointer);
> > + _wl_relative_pointer_release(window->relative_pointer);
> > + window->locked_pointer = NULL;
> > + window->relative_pointer = NULL;
> > + window->pointer_locked = false;
> > + window->locked_input = NULL;
>
> Is locked_input just a reference, or does it need to be destroyed or
> released as well?
It's just a reference. 'locked_input' here is the toytoolkit wl_seat object.
>
> > +}
> > +
> > +void
> > +widget_set_locked_pointer_cursor_hint(struct widget *widget,
> > + float x, float y)
> > +{
> > + struct window *window = widget->window;
> > +
> > + if (!window->locked_pointer)
> > + return;
> > +
> > + _wl_locked_pointer_set_cursor_position_hint(window->locked_pointer,
> > + wl_fixed_from_double(x),
> > + wl_fixed_from_double(y));
> > +}
> > +
> > +static void
> > +confined_pointer_confined(void *data,
> > + struct _wl_confined_pointer *confined_pointer)
> > +{
> > + struct input *input = data;
> > + struct window *window = input->pointer_focus;
> > +
> > + if (window->pointer_confined_handler) {
> > + window->pointer_confined_handler(window,
> > + input,
> > + window->user_data);
> > + }
> > + window->confined = true;
> > +}
> > +
> > +static void
> > +confined_pointer_unconfined(void *data,
> > + struct _wl_confined_pointer *confined_pointer)
> > +{
> > + struct input *input = data;
> > + struct window *window = input->pointer_focus;
> > +
> > + window_unconfine_pointer(window);
> > +
> > + if (window->pointer_unconfined_handler) {
> > + window->pointer_unconfined_handler(window,
> > + input,
> > + window->user_data);
> > + }
> > + window->confined = false;
> > +}
> > +
> > +static const struct _wl_confined_pointer_listener confined_pointer_listener = {
> > + confined_pointer_confined,
> > + confined_pointer_unconfined,
> > +};
> > +
> > +int
> > +window_confine_pointer_to_widget(struct window *window,
> > + struct widget *widget,
> > + struct input *input)
> > +{
> > + struct _wl_pointer_lock *pointer_lock = window->display->pointer_lock;
> > + struct _wl_confined_pointer *confined_pointer;
> > + struct wl_compositor *compositor = window->display->compositor;
> > + struct wl_region *region = NULL;
> > +
> > + if (!window->display->pointer_lock)
> > + return -1;
> > +
> > + if (window->locked_pointer)
> > + return -1;
> > +
> > + if (window->confined_pointer)
> > + return -1;
> > +
> > + if (!input->pointer)
> > + return -1;
> > +
> > + if (widget) {
> > + region = wl_compositor_create_region(compositor);
> > + wl_region_add(region,
> > + widget->allocation.x,
> > + widget->allocation.y,
> > + widget->allocation.width,
> > + widget->allocation.height);
> > + }
> > +
> > + confined_pointer =
> > + _wl_pointer_lock_confine_pointer(pointer_lock,
> > + window->main_surface->surface,
> > + input->seat,
> > + region);
> > + if (region)
> > + wl_region_destroy(region);
> > +
> > + _wl_confined_pointer_add_listener(confined_pointer,
> > + &confined_pointer_listener,
> > + input);
> > +
> > + window->confined_pointer = confined_pointer;
> > + window->confined_widget = widget;
> > +
> > + return 0;
> > +}
> > +
> > +void
> > +window_unconfine_pointer(struct window *window)
> > +{
> > + if (!window->confined_pointer)
> > + return;
> > +
> > + _wl_confined_pointer_destroy(window->confined_pointer);
> > + window->confined_pointer = NULL;
> > + window->confined = false;
> > +}
> > +
> > +static void
> > surface_enter(void *data,
> > struct wl_surface *wl_surface, struct wl_output *wl_output)
> > {
> > @@ -5285,6 +5592,15 @@ registry_handle_global(void *data, struct wl_registry *registry, uint32_t id,
> > } else if (strcmp(interface, "wl_seat") == 0) {
> > d->seat_version = version;
> > display_add_input(d, id);
> > + } else if (strcmp(interface, "_wl_relative_pointer_manager") == 0) {
> > + d->relative_pointer_manager =
> > + wl_registry_bind(registry, id,
> > + &_wl_relative_pointer_manager_interface,
> > + 1);
> > + } else if (strcmp(interface, "_wl_pointer_lock") == 0) {
> > + d->pointer_lock = wl_registry_bind(registry, id,
> > + &_wl_pointer_lock_interface,
> > + 1);
> > } else if (strcmp(interface, "wl_shm") == 0) {
> > d->shm = wl_registry_bind(registry, id, &wl_shm_interface, 1);
> > wl_shm_add_listener(d->shm, &shm_listener, d);
> > diff --git a/clients/window.h b/clients/window.h
> > index 0686c3f..c62a6db 100644
> > --- a/clients/window.h
> > +++ b/clients/window.h
> > @@ -223,6 +223,29 @@ typedef void (*window_output_handler_t)(struct window *window, struct output *ou
> > typedef void (*window_state_changed_handler_t)(struct window *window,
> > void *data);
> >
> > +
> > +typedef void (*window_locked_pointer_motion_handler_t)(struct window *window,
> > + struct input *input,
> > + uint32_t time,
> > + float x, float y,
> > + void *data);
> > +
> > +typedef void (*locked_pointer_locked_handler_t)(struct window *window,
> > + struct input *input,
> > + void *data);
> > +
> > +typedef void (*locked_pointer_unlocked_handler_t)(struct window *window,
> > + struct input *input,
> > + void *data);
> > +
> > +typedef void (*confined_pointer_confined_handler_t)(struct window *window,
> > + struct input *input,
> > + void *data);
> > +
> > +typedef void (*confined_pointer_unconfined_handler_t)(struct window *window,
> > + struct input *input,
> > + void *data);
> > +
> > typedef void (*widget_resize_handler_t)(struct widget *widget,
> > int32_t width, int32_t height,
> > void *data);
> > @@ -352,6 +375,24 @@ void
> > window_damage(struct window *window, int32_t x, int32_t y,
> > int32_t width, int32_t height);
> >
> > +int
> > +window_lock_pointer(struct window *window, struct input *input);
> > +
> > +void
> > +window_unlock_pointer(struct window *window);
> > +
> > +void
> > +widget_set_locked_pointer_cursor_hint(struct widget *widget,
> > + float x, float y);
> > +
> > +int
> > +window_confine_pointer_to_widget(struct window *window,
> > + struct widget *widget,
> > + struct input *input);
> > +
> > +void
> > +window_unconfine_pointer(struct window *window);
> > +
> > cairo_surface_t *
> > window_get_surface(struct window *window);
> >
> > @@ -430,6 +471,27 @@ window_set_state_changed_handler(struct window *window,
> > window_state_changed_handler_t handler);
> >
> > void
> > +window_set_pointer_locked_handler(struct window *window,
> > + locked_pointer_locked_handler_t locked);
> > +
> > +void
> > +window_set_pointer_unlocked_handler(struct window *window,
> > + locked_pointer_unlocked_handler_t unlocked);
> > +
> > +void
> > +window_set_pointer_confined_handler(
> > + struct window *window, confined_pointer_confined_handler_t confined);
> > +
> > +void
> > +window_set_pointer_unconfined_handler(
> > + struct window *window,
> > + confined_pointer_unconfined_handler_t unconfined);
> > +
> > +void
> > +window_set_locked_pointer_motion_handler(
> > + struct window *window, window_locked_pointer_motion_handler_t handler);
> > +
> > +void
> > window_set_title(struct window *window, const char *title);
>
> Apart from some of the cosmetic bits I had questions on, I didn't spot
> anything else of concern. So in hopes of seeing point locking land
> post-release,
>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
Thanks for the review. I didn't make any changes, but will take the
liberty of adding your R-B to the next version anyway given what you
wrote above.
Jonas
More information about the wayland-devel
mailing list