[PATCH] Implement move surface key bindings.

Scott Moreau oreaus at gmail.com
Fri Feb 3 02:10:22 PST 2012


From: Scott Moreau <oreaus at gmail.com>
Date: Fri, Feb 3, 2012 at 2:58 AM
Subject: Re: [PATCH] Implement move surface key bindings.
To: Pekka Paalanen <ppaalanen at gmail.com>




On Fri, Feb 3, 2012 at 1:29 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Thu,  2 Feb 2012 11:46:17 -0700
> Scott Moreau <oreaus at gmail.com> wrote:
>
> > Super+F7 to initiate, arrow keys to move, Enter to accept and Esc to
> terminate. We need a way to move the cursor to an abitrary position such as
> a warp_pointer function, instead of just moving the pointer image. We also
> need a uniform way to set a pointer image outside of toytoolkit.
> > ---
>
> Instead of "outside of toytoolkit" I would say "in the compositor",
> since that is where we need it, all clients and toolkits are already
> covered.
>
> Just thinking out loud, wonder if the compositor should load the cursor
> images, or should we invent something wacky, like the desktop-shell
> loading the cursor images into surfaces and setting them to the
> compositor via special desktop-shell protocol...
> How could we animate such cursors... oh that's obvious, the frame
> callback would run only when such cursor is used, so desktop-shell
> client could drive the animation. After we have converted cursor
> images from wl_buffers to wl_surfaces in general. :-P
>
> >  src/compositor.c |    4 +-
> >  src/shell.c      |  151
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 152 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/compositor.c b/src/compositor.c
> > index ab184ac..ab92bd6 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -1411,9 +1411,7 @@ notify_key(struct wl_input_device *device,
> >               *k = key;
> >       }
> >
> > -     if (device->keyboard_focus_resource)
> > -             wl_resource_post_event(device->keyboard_focus_resource,
> > -                                    WL_INPUT_DEVICE_KEY, time, key,
> state);
> > +     device->grab->interface->key(device->grab, time, key, state);
> >  }
> >
> >  WL_EXPORT void
> > diff --git a/src/shell.c b/src/shell.c
> > index 168443b..c7e277e 100644
> > --- a/src/shell.c
> > +++ b/src/shell.c
> > @@ -114,6 +114,7 @@ struct weston_move_grab {
> >       struct wl_grab grab;
> >       struct weston_surface *surface;
> >       int32_t dx, dy;
> > +     int32_t origin_x, origin_y;
> >  };
> >
> >  struct rotate_grab {
> > @@ -183,10 +184,73 @@ move_grab_button(struct wl_grab *grab,
> >       }
> >  }
> >
> > +static void
> > +move_grab_key(struct wl_grab *grab,
> > +              uint32_t time, int32_t key, int32_t state)
> > +{
> > +     if (state == 0)
> > +             return;
> > +
> > +     struct wl_input_device *device = grab->input_device;
> > +     struct weston_move_grab *move = (struct weston_move_grab *) grab;
> > +     struct weston_surface *es = move->surface;
> > +     int32_t x_offset = 0;
> > +     int32_t y_offset = 0;
> > +     int increment = 20;
> > +
> > +     switch (key) {
> > +             case KEY_UP:
> > +                     y_offset -= increment;
> > +                     break;
> > +             case KEY_LEFT:
> > +                     x_offset -= increment;
> > +                     break;
> > +             case KEY_RIGHT:
> > +                     x_offset += increment;
> > +                     break;
> > +             case KEY_DOWN:
> > +                     y_offset += increment;
> > +                     break;
> > +             case KEY_ENTER:
> > +                     wl_input_device_end_grab(device, time);
> > +                     free(grab);
> > +                     return;
> > +             case KEY_ESC:
> > +                     weston_surface_configure(es,
> > +                                              move->origin_x,
> > +                                              move->origin_y,
> > +                                              es->geometry.width,
> es->geometry.height);
> > +                     /* FIXME: Actually warp pointer instead
> > +                      * of just moving the cursor image */
> > +                     notify_motion(device, time,
> > +                                                     abs(move->dx) +
> es->geometry.x,
> > +                                                     abs(move->dy) +
> es->geometry.y);
>
> abs()? That looks odd. Why abs()?
>

Because  move->dx is a negative offset. (see below)

>
> > +                     wl_input_device_end_grab(device, time);
> > +                     free(grab);
> > +                     return;
> > +             default:
> > +                     return;
> > +     }
> > +
> > +     if (x_offset || y_offset) {
>
> Cannot get here with both zero, so no need to test.
>

Indeed.

>
> > +             weston_surface_configure(es,
> > +                                      x_offset + es->geometry.x,
> > +                                      y_offset + es->geometry.y,
> > +                                      es->geometry.width,
> es->geometry.height);
> > +             /* FIXME: Actually warp pointer instead
> > +              * of just moving the cursor image.
> > +              * Also, need a way to set cursor image. */
> > +             notify_motion(device, time,
> > +                                             abs(move->dx) +
> es->geometry.x,
> > +                                             abs(move->dy) +
> es->geometry.y);
>
> Another strange abs().
>

See comments below.

>
> > +     }
> > +}
> > +
> >  static const struct wl_grab_interface move_grab_interface = {
> >       noop_grab_focus,
> >       move_grab_motion,
> >       move_grab_button,
> > +     move_grab_key,
> >  };
>
> What happens, if you start a move by dragging the window title bar with
> mouse, and press enter or esc?
> What if you click a mouse button, while doing a keyboard move?
> Is the behaviour as intended then?
>
> Regarding the latter, it is intended behavior. So far as the former is
concerned, I did not think about it but it does indeed have the same effect
as a key grab. I assume this is not desired behavior.

>  static int
> > @@ -212,6 +276,37 @@ weston_surface_move(struct weston_surface *es,
> >       return 0;
> >  }
> >
> > +static int
> > +weston_surface_keyboard_move(struct weston_surface *es,
> > +                 struct weston_input_device *wd, uint32_t time)
> > +{
> > +     struct weston_move_grab *move;
> > +
> > +     move = malloc(sizeof *move);
> > +     if (!move)
> > +             return -1;
> > +
> > +     move->grab.interface = &move_grab_interface;
> > +     move->dx = es->geometry.x - ((es->geometry.width / 2) +
> es->geometry.x);
>
> This simplifies to move->dx = -es->geometry.width / 2. And since you
> seem to take abs() of it everywhere, you could just write
>        move->dx = es->geometry.width / 2;
> and drop the abs() everywhere. Or is this because the pointer move
> wants it negative? In that case, just use a minus instead of abs().
>

Yes, you are right. This will have to be adjusted appropriately.

>
> > +     move->dy = es->geometry.y - ((es->geometry.height / 2) +
> es->geometry.y);
> > +     move->origin_x = es->geometry.x;
> > +     move->origin_y = es->geometry.y;
> > +     move->surface = es;
> > +
> > +     /* FIXME: Actually warp pointer instead
> > +      * of just moving the cursor image */
> > +     notify_motion(&wd->input_device, time,
> > +                                     abs(move->dx) + es->geometry.x,
> > +                                     abs(move->dy) + es->geometry.y);
>
> If the purpose here is to move the pointer (in global coordinates) to
> the center of the surface, you'd better use
> weston_surface_to_global(es, width/2, height/2, &x, &y).
>
> weston_surface_{to,from}_global() are the only way to properly convert
> between surface-local and global coordinates.
>

I was unaware of this function and it seems to make more sense.

>
> Maybe you would need to use this to compute move->dx,dy?
> Move->dx,dy are in global coordinate system, width and height are in
> surface-local coordinate system, so you need a conversion somewhere.
>



More information about the wayland-devel mailing list