[PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface
Pekka Paalanen
ppaalanen at gmail.com
Thu May 26 15:01:27 UTC 2016
On Thu, 7 Apr 2016 16:44:17 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
Hi,
the commit message could use an explanation of what the 'bool active'
will actually be useful for. What do we need the "is in active use" for?
The comment in the code does not really tell me either. This ties in
with my comments on patch 3.
The patch subject is misleading. There is no inhibition state tracking
at all in this patch. Instead, this patch is about tracking surface
activeness as defined by the shells.
> ---
> v3: Rename inhibit_screensaving to inhibit_idling
>
> desktop-shell/shell.c | 4 +++-
> fullscreen-shell/fullscreen-shell.c | 25 +++++++++++++++-------
> ivi-shell/ivi-shell.c | 4 +++-
> src/compositor.c | 42 +++++++++++++++++++++++++++++++++++++
> src/compositor.h | 27 +++++++++++++++++++++---
> src/input.c | 15 -------------
> tests/weston-test.c | 8 +++++--
> 7 files changed, 96 insertions(+), 29 deletions(-)
>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 780902d..6e49076 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -5055,7 +5055,9 @@ activate(struct desktop_shell *shell, struct weston_surface *es,
> * Leave fullscreen surfaces on unrelated outputs alone. */
> lower_fullscreen_layer(shell, shsurf->output);
>
> - weston_surface_activate(es, seat);
> + weston_surface_assign_keyboard(es, seat);
> + if (es != NULL)
'es' cannot be NULL.
> + weston_surface_activate(es);
>
> state = ensure_focus_state(shell, seat);
> if (state == NULL)
> diff --git a/fullscreen-shell/fullscreen-shell.c b/fullscreen-shell/fullscreen-shell.c
> index abc4e84..e1f8a63 100644
> --- a/fullscreen-shell/fullscreen-shell.c
> +++ b/fullscreen-shell/fullscreen-shell.c
> @@ -88,8 +88,11 @@ pointer_focus_changed(struct wl_listener *listener, void *data)
> {
> struct weston_pointer *pointer = data;
>
> - if (pointer->focus && pointer->focus->surface->resource)
> - weston_surface_activate(pointer->focus->surface, pointer->seat);
> + if (pointer->focus && pointer->focus->surface->resource) {
> + weston_surface_assign_keyboard(pointer->focus->surface, pointer->seat);
> + if (pointer->focus->surface != NULL)
pointer->focus->surface cannot be NULL.
> + weston_surface_activate(pointer->focus->surface);
> + }
> }
>
> static void
> @@ -118,7 +121,9 @@ seat_caps_changed(struct wl_listener *l, void *data)
> if (keyboard && keyboard->focus != NULL) {
> wl_list_for_each(fsout, &listener->shell->output_list, link) {
> if (fsout->surface) {
> - weston_surface_activate(fsout->surface, seat);
> + weston_surface_assign_keyboard(fsout->surface, seat);
> + if (fsout->surface != NULL)
Cannot be NULL.
> + weston_surface_activate(fsout->surface);
> return;
> }
> }
> @@ -703,8 +708,11 @@ fullscreen_shell_present_surface(struct wl_client *client,
> struct weston_keyboard *keyboard =
> weston_seat_get_keyboard(seat);
>
> - if (keyboard && !keyboard->focus)
> - weston_surface_activate(surface, seat);
> + if (keyboard && !keyboard->focus) {
> + weston_surface_assign_keyboard(surface, seat);
> + if (surface != NULL)
Cannot be NULL.
> + weston_surface_activate(surface);
> + }
> }
> }
> }
> @@ -754,8 +762,11 @@ fullscreen_shell_present_surface_for_mode(struct wl_client *client,
> struct weston_keyboard *keyboard =
> weston_seat_get_keyboard(seat);
>
> - if (keyboard && !keyboard->focus)
> - weston_surface_activate(surface, seat);
> + if (keyboard && !keyboard->focus) {
> + weston_surface_assign_keyboard(surface, seat);
> + if (surface != NULL)
Cannot be NULL.
> + weston_surface_activate(surface);
> + }
> }
> }
>
> diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> index a767ccf..59f5656 100644
> --- a/ivi-shell/ivi-shell.c
> +++ b/ivi-shell/ivi-shell.c
> @@ -425,7 +425,9 @@ activate_binding(struct weston_seat *seat,
> if (get_ivi_shell_surface(main_surface) == NULL)
> return;
>
> - weston_surface_activate(focus, seat);
> + weston_surface_assign_keyboard(focus, seat);
> + if (focus != NULL)
Cannot be NULL.
> + weston_surface_activate(focus);
> }
>
> static void
> diff --git a/src/compositor.c b/src/compositor.c
> index 83cabf7..9531a0a 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -612,6 +612,9 @@ weston_surface_create(struct weston_compositor *compositor)
> weston_matrix_init(&surface->buffer_to_surface_matrix);
> weston_matrix_init(&surface->surface_to_buffer_matrix);
>
> + surface->active = false;
> + surface->inhibit_idling = false;
> +
> return surface;
> }
>
> @@ -3422,6 +3425,45 @@ weston_surface_copy_content(struct weston_surface *surface,
> src_x, src_y, width, height);
> }
>
> +/** Sets the keyboard focus to the given surface
> + */
> +WL_EXPORT void
> +weston_surface_assign_keyboard(struct weston_surface *surface,
> + struct weston_seat *seat)
This should be called weston_seat_set_keyboard_focus(seat, surface).
Keyboard focus is a property of the seat.
> +{
> + struct weston_compositor *compositor = seat->compositor;
> + struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> +
> + if (keyboard) {
> + weston_keyboard_set_focus(keyboard, surface);
> + wl_data_device_set_keyboard_focus(seat);
I wonder if wl_data_device_set_keyboard_focus() is a misnomer. Its
purpose is to offer the selection to the newly activated surface, so in
that respect it should belong in weston_surface_activate() and not be
tied to the keyboard in any way. But, as it currently is inherently
tied to the keyboard, it needs to be here as it is.
> + }
> +
> + wl_signal_emit(&compositor->activate_signal, surface);
Why is sending the activate_signal here and not part of
weston_surface_activate()?
> +}
> +
> +/** Set surface as considered 'active' by the shell.
> + */
> +WL_EXPORT void
> +weston_surface_activate(struct weston_surface *surface)
> +{
> + surface->active = true;
> +}
> +
> +/** Set surface as no longer considered 'active' by the shell.
> + */
> +WL_EXPORT void
> +weston_surface_deactivate(struct weston_surface *surface)
> +{
> + surface->active = false;
> +}
There was not a single call to weston_surface_deactivate() in this
patch. IOW, half of this patch is missing! See comments to patch 4.
That is, if surface->active is meant to essentially be equivalent to
keyboard focus if a keyboard existed?
> +
> +WL_EXPORT bool
> +weston_surface_is_active(struct weston_surface *surface)
> +{
> + return surface->active;
> +}
This is also unused, but that's ok, I assume no-one is yet interested
in it. But missing the deactivate is a real problem, because not using
it will leave stale state all over.
> +
> static void
> subsurface_set_position(struct wl_client *client,
> struct wl_resource *resource, int32_t x, int32_t y)
> diff --git a/src/compositor.h b/src/compositor.h
> index df8ef2d..d8d5368 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -1037,6 +1037,20 @@ struct weston_surface {
> const char *role_name;
>
> struct weston_timeline_object timeline;
> +
> + /*
> + * A shell-specific indicator that the surface is in immediate
> + * use by the user. For example, the surface might have keyboard
> + * focus.
> + */
> + bool active;
> +
> + /*
> + * Indicates the surface prefers no screenblanking, screensaving,
> + * or other automatic obscurement to kick in while the surface is
> + * considered "active" by the shell.
> + */
> + bool inhibit_idling;
This member is added, but not used at all, not even in any added unused
functions. Therefore it does not belong in this patch, it belongs in the
patch that starts using it.
> };
>
> struct weston_subsurface {
> @@ -1122,9 +1136,6 @@ int
> weston_spring_done(struct weston_spring *spring);
>
> void
> -weston_surface_activate(struct weston_surface *surface,
> - struct weston_seat *seat);
> -void
> notify_motion(struct weston_seat *seat, uint32_t time,
> struct weston_pointer_motion_event *event);
> void
> @@ -1402,6 +1413,16 @@ weston_surface_copy_content(struct weston_surface *surface,
> int src_x, int src_y,
> int width, int height);
>
> +void
> +weston_surface_assign_keyboard(struct weston_surface *surface,
> + struct weston_seat *seat);
> +void
> +weston_surface_activate(struct weston_surface *surface);
> +void
> +weston_surface_deactivate(struct weston_surface *surface);
> +bool
> +weston_surface_is_active(struct weston_surface *surface);
> +
> struct weston_buffer *
> weston_buffer_from_resource(struct wl_resource *resource);
>
> diff --git a/src/input.c b/src/input.c
> index a3d982e..6e35105 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -1206,21 +1206,6 @@ notify_motion_absolute(struct weston_seat *seat,
> }
>
> WL_EXPORT void
> -weston_surface_activate(struct weston_surface *surface,
> - struct weston_seat *seat)
> -{
> - struct weston_compositor *compositor = seat->compositor;
> - struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> -
> - if (keyboard) {
> - weston_keyboard_set_focus(keyboard, surface);
> - wl_data_device_set_keyboard_focus(seat);
> - }
> -
> - wl_signal_emit(&compositor->activate_signal, surface);
> -}
It is a bit hard to see if this function was allowed to be called with
NULL, so I just checked all the callsites replaced in this patch.
> -
> -WL_EXPORT void
> notify_button(struct weston_seat *seat, uint32_t time, int32_t button,
> enum wl_pointer_button_state state)
> {
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index 03e2c54..15cd07f 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -181,13 +181,17 @@ activate_surface(struct wl_client *client, struct wl_resource *resource,
> seat = get_seat(test);
> keyboard = weston_seat_get_keyboard(seat);
> if (surface) {
> - weston_surface_activate(surface, seat);
> + weston_surface_assign_keyboard(surface, seat);
> + if (surface != NULL)
> + weston_surface_activate(surface);
Redundant check, surface is always non-NULL.
> notify_keyboard_focus_in(seat, &keyboard->keys,
> STATE_UPDATE_AUTOMATIC);
> }
> else {
> notify_keyboard_focus_out(seat);
> - weston_surface_activate(surface, seat);
> + weston_surface_assign_keyboard(surface, seat);
> + if (surface != NULL)
> + weston_surface_activate(surface);
Redundant check, surface is always NULL.
> }
> }
>
Why is there no call to deactivate for the previously active surface
here? What will call deactivate as necessary?
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160526/130df9ee/attachment.sig>
More information about the wayland-devel
mailing list