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

Scott Moreau oreaus at gmail.com
Fri Jun 1 00:57:36 PDT 2012


On Fri, Jun 1, 2012 at 1:30 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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.
>

Ok thanks for pointing this out.


>
> >  {
> >       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.
>

Yes, I don't really understand all this wl_fixed_t stuff and it's somewhat
annoying.


>
> 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.
>

Ok thanks, I have to rework this patch anyway against latest commits.


>
> 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...
>

Yes again, annoying.


>
> >
> >       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
>



Thanks,

Scott
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120601/b93d6ef7/attachment-0001.htm>


More information about the wayland-devel mailing list