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

[Daniel Stone]

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.

>> @@ -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?).

Thanks for the review!

Cheers,
Daniel