[PATCH weston v2] Text cursor position protocol follow-up.

Pekka Paalanen ppaalanen at gmail.com
Fri Jun 1 00:30:33 PDT 2012


On Thu, 31 May 2012 14:02:48 -0600
Scott Moreau <oreaus at gmail.com> wrote:

> * Drop user input detection since cursor position may change when pasting text,
> for example.
> * Use fixed type in protocol.

Hi Scott,

some inline comments I below.

> ---
> 
> Changed int to fixed in protocol/text-cursor-position.xml
> 
>  clients/window.c                  |   15 +++++----------
>  clients/window.h                  |    2 +-
>  protocol/text-cursor-position.xml |    4 ++--
>  src/compositor.c                  |   11 ++++++-----
>  src/compositor.h                  |    6 ++++--
>  src/text-cursor-position.c        |    2 +-
>  6 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index 3ef648e..74fc5c1 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -151,7 +151,6 @@ struct window {
>  	int resize_needed;
>  	int type;
>  	int transparent;
> -	int send_cursor_position;
>  	struct input *keyboard_device;
>  	enum window_buffer_type buffer_type;
>  
> @@ -1826,9 +1825,6 @@ keyboard_handle_key(void *data, struct wl_keyboard *keyboard,
>  	if (!window || window->keyboard_device != input)
>  		return;
>  
> -	if (state)
> -		window->send_cursor_position = 1;
> -
>  	num_syms = xkb_key_get_syms(d->xkb.state, code, &syms);
>  	xkb_state_update_key(d->xkb.state, code,
>  			     state ? XKB_KEY_DOWN : XKB_KEY_UP);
> @@ -2616,18 +2612,18 @@ window_get_title(struct window *window)
>  }
>  
>  void
> -window_set_text_cursor_position(struct window *window, int32_t x, int32_t y)
> +window_set_text_cursor_position(struct window *window, uint32_t x, uint32_t y)

Why the change in argument types?
I think we have all integer coordinates always as signed, regardless of
valid domain.

>  {
>  	struct text_cursor_position *text_cursor_position =
>  					window->display->text_cursor_position;
>  
> -	if (!window->send_cursor_position || !text_cursor_position)
> +	if (!text_cursor_position)
>  		return;
>  
>  	text_cursor_position_notify(text_cursor_position,
> -						window->surface, x, y);
> -
> -	window->send_cursor_position = 0;
> +						window->surface,
> +						wl_fixed_from_int(x),
> +						wl_fixed_from_int(y));
>  }
>  
>  void
> @@ -2716,7 +2712,6 @@ window_create_internal(struct display *display, struct window *parent)
>  	window->allocation.height = 0;
>  	window->saved_allocation = window->allocation;
>  	window->transparent = 1;
> -	window->send_cursor_position = 0;
>  	window->type = TYPE_NONE;
>  	window->input_region = NULL;
>  	window->opaque_region = NULL;
> diff --git a/clients/window.h b/clients/window.h
> index a8537b3..09e4dc8 100644
> --- a/clients/window.h
> +++ b/clients/window.h
> @@ -308,7 +308,7 @@ const char *
>  window_get_title(struct window *window);
>  
>  void
> -window_set_text_cursor_position(struct window *window, int32_t x, int32_t y);
> +window_set_text_cursor_position(struct window *window, uint32_t x, uint32_t y);
>  
>  int
>  widget_set_tooltip(struct widget *parent, char *entry, float x, float y);
> diff --git a/protocol/text-cursor-position.xml b/protocol/text-cursor-position.xml
> index dbeda72..0fbc54e 100644
> --- a/protocol/text-cursor-position.xml
> +++ b/protocol/text-cursor-position.xml
> @@ -3,8 +3,8 @@
>    <interface name="text_cursor_position" version="1">
>      <request name="notify">
>        <arg name="surface" type="object" interface="wl_surface"/>
> -      <arg name="x" type="uint"/>
> -      <arg name="y" type="uint"/>
> +      <arg name="x" type="fixed"/>
> +      <arg name="y" type="fixed"/>
>      </request>
>    </interface>
>  
> diff --git a/src/compositor.c b/src/compositor.c
> index a36ccd5..fbfab6e 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -2446,14 +2446,15 @@ weston_output_destroy(struct weston_output *output)
>  
>  WL_EXPORT void
>  weston_text_cursor_position_notify(struct weston_surface *surface,
> -						int32_t cur_pos_x,
> -						int32_t cur_pos_y)
> +						wl_fixed_t cur_pos_fx,
> +						wl_fixed_t cur_pos_fy)
>  {
>  	struct weston_output *output;
> -	int32_t global_x, global_y;
> +	wl_fixed_t global_x, global_y;
>  
> -	weston_surface_to_global(surface, cur_pos_x, cur_pos_y,
> -						&global_x, &global_y);
> +	weston_surface_to_global(surface, wl_fixed_to_int(cur_pos_fx),
> +					  wl_fixed_to_int(cur_pos_fy),
> +					  &global_x, &global_y);

That is wrong, because global_x is wl_fixed_t and
weston_surface_to_global() expects an int32_t. Therefore the result
will be off by a factor of 256. If this works for you, then there
probably is another similar confusion between int and fixed elsewhere.

You should use weston_surface_to_global_fixed() instead, dropping the
wl_fixed_to_int() calls, which throw away precision - the reason why
wl_fixed_t exists in the first place.

This would have been cought by the compiler, if wl_fixed_t just was a
type-safe type and not something that implicitly casts to any
integer...

>  
>  	wl_list_for_each(output, &surface->compositor->output_list, link)
>  		if (output->zoom.active &&
> diff --git a/src/compositor.h b/src/compositor.h
> index 09cd215..63ad3a6 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -567,9 +567,11 @@ void
>  weston_compositor_shutdown(struct weston_compositor *ec);
>  void
>  weston_output_update_zoom(struct weston_output *output,
> -						int x, int y, uint32_t type);
> +						wl_fixed_t x, wl_fixed_t y,
> +						uint32_t type);
>  void
> -weston_text_cursor_position_notify(struct weston_surface *surface, int x, int y);
> +weston_text_cursor_position_notify(struct weston_surface *surface,
> +						wl_fixed_t x, wl_fixed_t y);
>  void
>  weston_output_update_matrix(struct weston_output *output);
>  void
> diff --git a/src/text-cursor-position.c b/src/text-cursor-position.c
> index 6f46636..ef1085a 100644
> --- a/src/text-cursor-position.c
> +++ b/src/text-cursor-position.c
> @@ -37,7 +37,7 @@ static void
>  text_cursor_position_notify(struct wl_client *client,
>  			    struct wl_resource *resource,
>  			    struct wl_resource *surface_resource,
> -			    uint32_t x, uint32_t y)
> +			    wl_fixed_t x, wl_fixed_t y)
>  {
>  	weston_text_cursor_position_notify((struct weston_surface *) surface_resource, x, y);
>  }


Thanks,
pq


More information about the wayland-devel mailing list