[PATCH weston 4/5] Make use of new wl_cursors

[Kristian Høgsberg]

On Wed, Jul 25, 2012 at 9:11 AM, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Ander,
>
> On 25 July 2012 12:57, Ander Conselvan de Oliveira <conselvan2 at gmail.com> wrote:
>> On 07/23/2012 09:55 PM, Daniel Stone wrote:
>>> @@ -1394,8 +1414,13 @@ surface_attach(struct wl_client *client,
>>>
>>>         weston_surface_attach(&es->surface, buffer);
>>>
>>> -       if (buffer && es->configure)
>>> +       if (!buffer)
>>> +               return;
>>> +
>>> +       if (es->configure)
>>>                 es->configure(es, sx, sy);
>>> +
>>> +       check_cursor_configure(es, sx, sy);
>>>   }
>>
>>
>> It would be better to have an appropriate es->configure set for every
>> surface used as a cursor and avoid doing this check for every single attach
>> in every single surface. This can be done in cursor_new and we should fail
>> if the surface already has es->configure set.
>
> It's fair.  To be honest, although reusing surface configures is a
> cute hack, I'm not massively happy with it right now, and it did cause
> me some problems during the implementation.  I'm wondering if a
> wl_cursor_surface (akin to wl_shell_surface) that lets you explicitly
> set hotspots/etc, wouldn't be a better idea in the long run.

We're using surface->configure the way it's supposed to be used.  A
raw wl_surface (as just created from wl_compositor.create_surface)
doesn't have a well defined way to map into the scenegraph and
attaching buffers doesn't do anything.  Before it can show up, you
have to assign it a 'role', which can be done in a number of ways:
create a wl_shell_surface for it, wl_pointer.set_cursor to make it a
cursor, set it as background or panel/etc using
desktop_shell.set_background/panel/etc.

In weston, the different roles are implemented by setting
surface->configure.  You can think of surface->configure as a
interface, vtable or class, it's just that we only need that one
function right now so it's no a full interface struct.  Once
surface->configure is set, we know how to place the surface in the
stack, where it goes on the screen and what should happen when the
client resizes it.

>>> @@ -2177,103 +2281,32 @@ notify_touch(struct wl_seat *seat, uint32_t time,
>>> -static void
>>>   pointer_set_cursor(struct wl_client *client, struct wl_resource
>>> *resource,
>>>                    uint32_t serial, struct wl_resource *surface_resource,
>>>                    int32_t x, int32_t y)
>>>   {
>>>         struct weston_seat *seat = resource->data;
>>> -       struct weston_surface *surface = NULL;
>>> -
>>> -       if (surface_resource)
>>> -               surface = container_of(surface_resource->data,
>>> -                                      struct weston_surface, surface);
>>> +       struct weston_surface *surface;
>>>
>>> -       if (serial < seat->seat.pointer->focus_serial)
>>> +       if (!surface_resource) {
>>> +               wl_pointer_set_cursor_icon(&seat->pointer, NULL, 0, 0);
>>>                 return;
>>
>>
>> You are effectively allowing any client to hide the cursor at any moment
>> regardless of where the focus is.
>
> Oops, good catch.  Amended locally, thanks!
>
>>> -               if (surface->configure) {
>>> -                       wl_resource_post_error(&surface->surface.resource,
>>> -
>>> WL_DISPLAY_ERROR_INVALID_OBJECT,
>>> -                                              "surface->configure already
>>> "
>>> -                                              "set");
>>> -                       return;
>>
>>
>> Removing this makes it possible to make any surface a cursor, such as a
>> fullscreen shell surface. We don't want that.
>>
>> Anyway, this whole logic is there because the protocol allows a client to
>> change the cursor surface if it has focus or is the owner of the current
>> pointer surface. That is the reason for the
>>
>>
>>  if (surface && surface != seat->sprite)
>>
>> check. This way, a client that lost the focus can still update the hotspot.
>> If you changes, this is not possible anymore.
>
> OK, good point.  I'll resubmit using the old ->configure override
> logic.  I still think in the long run we'd possibly be best served by
> a wl_cursor_surface though ... (krh?).

wl_cursor_surface is just different protocol, in the compositor
wl_cursor_surface would also be implemented by setting
surface->configure.  We added wl_shell_surface, because we had a lot
of requests on wl_shell that went like: wl_shell.set_fullscreen(shell,
surface), ie, took the shell and a surface, and in effect were surface
requests.  So the wl_shell_surface is almost just protocol syntatic
sugar, and I'm not always sure it actually makes things clearer.

Kristian