[PATCH weston 1/3] compositor: Update input region when updating surface transfomations

[Jonas Ådahl]

On Thu, Nov 15, 2012 at 8:52 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 14 Nov 2012 23:39:51 +0100
> Jonas Ådahl <jadahl at gmail.com> wrote:
>
>> If the compositor updated the geometry of a surface, the input region
>> would still represent the original geometry. By intersecting the input
>> region specified by the client, if any, by the new geometry when
>> updating surface transformations the active input region can be kept up
>> to date.
>
> Did you hit this problem when a surface radically changes size in a
> single step, leaving the input region obviously wrong? Or something
> more subtle? That would be nice to mention.

The problem happens if the compositor (in particular in event-test.c)
calls weston_surface_configure() and then tries to repick a surface.
When repicking, the input region will be the same as it was before the
reconfiguration, which potentially could be the rect 0x0, resulting it
wont be repicked by the compositor.

>
>> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
>> ---
>>  src/compositor.c |   23 +++++++++++++++++------
>>  src/compositor.h |    1 +
>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/compositor.c b/src/compositor.c
>> index 83bd244..75c66ab 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c
>> @@ -249,6 +249,7 @@ weston_surface_create(struct weston_compositor *compositor)
>>       pixman_region32_init(&surface->opaque);
>>       pixman_region32_init(&surface->clip);
>>       region_init_infinite(&surface->input);
>> +     region_init_infinite(&surface->full_input);
>>       pixman_region32_init(&surface->transform.opaque);
>>       wl_list_init(&surface->frame_callback_list);
>>
>> @@ -508,6 +509,17 @@ weston_surface_update_transform_enable(struct weston_surface *surface)
>>       return 0;
>>  }
>>
>> +static void
>> +weston_surface_update_input_region(struct weston_surface *surface)
>> +{
>> +     pixman_region32_fini(&surface->input);
>> +     pixman_region32_init_rect(&surface->input, 0, 0,
>> +                               surface->geometry.width,
>> +                               surface->geometry.height);
>> +     pixman_region32_intersect(&surface->input,
>> +                               &surface->input, &surface->full_input);
>> +}
>> +
>>  WL_EXPORT void
>>  weston_surface_update_transform(struct weston_surface *surface)
>>  {
>> @@ -522,6 +534,8 @@ weston_surface_update_transform(struct weston_surface *surface)
>>       pixman_region32_fini(&surface->transform.opaque);
>>       pixman_region32_init(&surface->transform.opaque);
>>
>> +     weston_surface_update_input_region(surface);
>> +
>>       /* transform.position is always in transformation_list */
>>       if (surface->geometry.transformation_list.next ==
>>           &surface->transform.position.link &&
>> @@ -791,6 +805,7 @@ destroy_surface(struct wl_resource *resource)
>>       pixman_region32_fini(&surface->opaque);
>>       pixman_region32_fini(&surface->clip);
>>       pixman_region32_fini(&surface->input);
>> +     pixman_region32_fini(&surface->full_input);
>>
>>       wl_list_for_each_safe(cb, next, &surface->frame_callback_list, link)
>>               wl_resource_destroy(&cb->resource);
>> @@ -1286,12 +1301,8 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
>>       pixman_region32_fini(&opaque);
>>
>>       /* wl_surface.set_input_region */
>> -     pixman_region32_fini(&surface->input);
>> -     pixman_region32_init_rect(&surface->input, 0, 0,
>> -                               surface->geometry.width,
>> -                               surface->geometry.height);
>> -     pixman_region32_intersect(&surface->input,
>> -                               &surface->input, &surface->pending.input);
>> +     pixman_region32_copy(&surface->full_input, &surface->pending.input);
>> +     weston_surface_update_input_region(surface);
>
> Hi,
>
> just wondering, do we actually want to call
> weston_surface_update_input_region() here?
>
> If we think about what's on screen, that won't change until repaint, so
> wouldn't it be logical to actually update the input region only at
> repaint? Yeah, this is a change from how it was before, but I think
> this could be more correct.

This would work, I suppose. Practically, the test cases would need to
be changed to require a redraw before continuing after resizing
surface so that later motion events have the expected effect.

>
> If the server sends input to the client surface after
> wl_surface.commit, but before repaint, then from user perspective the
> input corresponds to the old surface contents. Hence, also clients
> should be expecting input relative to the old contents until they
> actually get the frame callback.

With this patch, if a client sends input to the surface after commit,
the previously set input will still be the one in use, until next
wl_surface.commit even if the compositor changes the dimensions. If a
client never set any input region, the coordinates it receives might
be confusing, though, as it'll be whatever the compositor thinks is
the surface size. If a client cares about this, maybe this is good
enough to have it have control on what it receives by setting input
regions when creating?

Looking in shell.c where clients are notified of surface changes via
wl_shell_surface_configure() I can't see any obvious correlation with
what has been painted or not. Do you think we should change this as
well, so that clients will be notified of dimension changes after the
frame callback?

>
> Or should we just not care, since even pointer motion triggers a
> repaint, and it's practically impossible for a user to tell the
> difference between (to react to the change, actually) whether the input
> was for this or previous content?
>
> Well... like I said, just wondering.

I think either way would practically work, but would be good to
document what the client can expect and how server implementers should
behave.

>
>
> Thanks,
> pq
>

Jonas