[PATCH weston v3] libweston: Position layers in an absolute way
Pekka Paalanen
ppaalanen at gmail.com
Mon Dec 5 15:29:01 UTC 2016
On Mon, 11 Jul 2016 11:29:40 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:
> From: Quentin Glidic <sardemff7+git at sardemff7.net>
>
> Currently, layers’ order depends on the module loading order and it does
> not survive runtime modifications (like shell locking/unlocking).
> With this patch, modules can safely add their own layer at the expected
> position in the stack, with runtime persistence.
>
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>
> v3:
> - Added weston_layer_unset_position
> - HIDDEN is now still rendered
> (Feature needed by Giulio Camuffo)
> - weston_layer now stores the weston_compositor pointer to avoid its
> need in set_position
> - Reordered weston_layer members (we break the ABI anyway)
> - weston_layer_init now only initialize the struct, you must
> call set_position to add it to the layer list
> - BACKGROUND is now 2 instead of 1, as these values are mainly meant
> for libweston modules. The compositor should simply use
> (BACKGROUND - 1) if it wants to support such modules with
> a fallback surface
> (Suggestion by Bill Spitzak)
>
> v2:
> - Tiny commit message addition: added runtime behaviour comment.
> - Reworked (a lot) the enum comment to explain further the position
> values and their actual expected scope. I hope their are clear enough.
> Here are the biggest changes:
> - Added a BOTTOM_UI value for widgets and conky-like applications.
> - Changed BACKGROUND to be 1, as nothing should be under it
> - Added a comment about mandatory background
>
> desktop-shell/input-panel.c | 6 ++--
> desktop-shell/shell.c | 64 +++++++++++++++++----------------
> fullscreen-shell/fullscreen-shell.c | 4 ++-
> ivi-shell/input-panel-ivi.c | 6 ++--
> ivi-shell/ivi-layout.c | 4 ++-
> ivi-shell/ivi-shell.c | 2 +-
> libweston/compositor.c | 52 ++++++++++++++++++++++++---
> libweston/compositor.h | 70 +++++++++++++++++++++++++++++++++++--
> tests/weston-test.c | 3 +-
> 9 files changed, 163 insertions(+), 48 deletions(-)
>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index c72f801..bdaad87 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
I looks like these items still remain:
desktop-shell/shell.c=1048=finish_workspace_change_animation(struct desktop_shell *shell,
desktop-shell/shell.c:1068: wl_list_remove(&shell->workspaces.anim_from->layer.link);
desktop-shell/shell.c=1123=animate_workspace_change(struct desktop_shell *shell,
desktop-shell/shell.c:1150: wl_list_insert(from->layer.link.prev, &to->layer.link);
desktop-shell/shell.c=1160=update_workspace(struct desktop_shell *shell, unsigned int index,
desktop-shell/shell.c:1164: wl_list_insert(&from->layer.link, &to->layer.link);
desktop-shell/shell.c:1165: wl_list_remove(&from->layer.link);
desktop-shell/shell.c=1255=take_surface_to_workspace_by_seat(struct desktop_shell *shell,
desktop-shell/shell.c:1289: wl_list_remove(&to->layer.link);
desktop-shell/shell.c:1290: wl_list_insert(from->layer.link.prev, &to->layer.link);
They are all manipulating the weston_layer::link, so it seems to me
they should be fixed.
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 771f1c9..9d6bbf6 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -2402,14 +2402,52 @@ weston_layer_entry_remove(struct weston_layer_entry *entry)
> entry->layer = NULL;
> }
>
> +
> +/** Initialize the weston_layer struct.
> + *
> + * \param compositor The compositor instance
> + * \param layer The layer to initialize
> + */
> WL_EXPORT void
> -weston_layer_init(struct weston_layer *layer, struct wl_list *below)
> +weston_layer_init(struct weston_compositor *compositor,
> + struct weston_layer *layer)
> {
> + layer->compositor = compositor;
> + wl_list_init(&layer->link);
> wl_list_init(&layer->view_list.link);
> layer->view_list.layer = layer;
> weston_layer_set_mask_infinite(layer);
> - if (below != NULL)
> - wl_list_insert(below, &layer->link);
> +}
> +
> +/** Sets the position of the layer in the layer list.
> + *
> + * \param layer The layer to modify
> + * \param position The position the layer will be placed at
This should document that calling set_position twice without an unset
in between is not allowed.
> + */
> +WL_EXPORT void
> +weston_layer_set_position(struct weston_layer *layer,
> + enum weston_layer_position position)
> +{
> + struct weston_layer *below;
> +
> + layer->position = position;
> + wl_list_for_each_reverse(below, &layer->compositor->layer_list, link) {
> + if (below->position >= layer->position) {
> + wl_list_insert(&below->link, &layer->link);
> + return;
> + }
> + }
> + wl_list_insert(layer->compositor->layer_list.next, &layer->link);
I think this should not have .next, should it? Now it's adding the new
layer below the top-most layer when it should become the top-most
layer, no?
> +}
> +
> +/** Hide a layer by taking it off the layer list.
> + *
> + * \param layer The layer to hide
This should document that calling this on a layer that is "not on the
list" is not allowed.
However, since init() does init the link, I wonder if you meant these
to be safe instead of disallow repeated calls?
> + */
> +WL_EXPORT void
> +weston_layer_unset_position(struct weston_layer *layer)
> +{
> + wl_list_remove(&layer->link);
> }
>
> WL_EXPORT void
> @@ -4725,8 +4763,12 @@ weston_compositor_create(struct wl_display *display, void *user_data)
> loop = wl_display_get_event_loop(ec->wl_display);
> ec->idle_source = wl_event_loop_add_timer(loop, idle_handler, ec);
>
> - weston_layer_init(&ec->fade_layer, &ec->layer_list);
> - weston_layer_init(&ec->cursor_layer, &ec->fade_layer.link);
> + weston_layer_init(ec, &ec->fade_layer);
> + weston_layer_init(ec, &ec->cursor_layer);
> +
> + weston_layer_set_position(&ec->fade_layer, WESTON_LAYER_POSITION_FADE);
> + weston_layer_set_position(&ec->cursor_layer,
> + WESTON_LAYER_POSITION_CURSOR);
>
> weston_compositor_add_debug_binding(ec, KEY_T,
> timeline_key_binding_handler, ec);
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index 557d2f5..d6e35e0 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -605,10 +605,68 @@ struct weston_layer_entry {
> struct weston_layer *layer;
> };
>
> +/**
> + * Higher value means higher in the stack.
> + *
> + * These values are based on well-known concepts in a classic desktop
> + * environment. Third-party modules based on libweston are encouraged to use
> + * them to integrate better with other projects.
> + *
> + * A fully integrated environment can use any value, based on these or not,
> + * at their discretion.
> + */
> +enum weston_layer_position {
> + /*
> + * Special value to make the layer invisible and still rendered.
> + * This is used by compositors wanting e.g. minimized surfaces to still
> + * receive frame callbacks.
> + */
> + WESTON_LAYER_POSITION_HIDDEN = 0x00000000,
> +
> + /*
> + * There should always be a background layer with a surface covering
> + * the visible area.
> + *
> + * If the compositor handles the background itself, it should use
> + * BACKGROUND.
> + *
> + * If the compositor supports runtime-loadable modules to set the
> + * background, it should put a solid color surface at (BACKGROUND - 1)
> + * and modules must use BACKGROUND.
> + */
> + WESTON_LAYER_POSITION_BACKGROUND = 0x00000002,
> +
> + /* For "desktop widgets" and applications like conky. */
> + WESTON_LAYER_POSITION_BOTTOM_UI = 0x30000000,
> +
> + /* For regular applications, only one layer should have this value
> + * to ensure proper stacking control. */
> + WESTON_LAYER_POSITION_NORMAL = 0x50000000,
> +
> + /* For desktop UI, like panels. */
> + WESTON_LAYER_POSITION_UI = 0x80000000,
> +
> + /* For fullscreen applications that should cover UI. */
> + WESTON_LAYER_POSITION_FULLSCREEN = 0xb0000000,
> +
> + /* For special UI like on-screen keyboard that fullscreen applications
> + * will need. */
> + WESTON_LAYER_POSITION_TOP_UI = 0xe0000000,
> +
> + /* For the lock surface. */
> + WESTON_LAYER_POSITION_LOCK = 0xffff0000,
> +
> + /* Values reserved for libweston internal usage */
> + WESTON_LAYER_POSITION_CURSOR = 0xfffffffe,
> + WESTON_LAYER_POSITION_FADE = 0xffffffff,
> +};
> +
> struct weston_layer {
> - struct weston_layer_entry view_list;
> - struct wl_list link;
> + struct weston_compositor *compositor;
> + struct wl_list link; /* weston_compositor::layer_list */
> + enum weston_layer_position position;
I'm not sure this should be 'enum' because the enumeration does not
actually list all possible values.
> pixman_box32_t mask;
> + struct weston_layer_entry view_list;
> };
>
> struct weston_plane {
> @@ -1222,7 +1280,13 @@ weston_layer_entry_insert(struct weston_layer_entry *list,
> void
> weston_layer_entry_remove(struct weston_layer_entry *entry);
> void
> -weston_layer_init(struct weston_layer *layer, struct wl_list *below);
> +weston_layer_init(struct weston_compositor *compositor,
> + struct weston_layer *layer);
> +void
> +weston_layer_set_position(struct weston_layer *layer,
> + enum weston_layer_position position);
> +void
> +weston_layer_unset_position(struct weston_layer *layer);
>
> void
> weston_layer_set_mask(struct weston_layer *layer, int x, int y, int width, int height);
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index 09d8b5e..4de42fc 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -595,7 +595,8 @@ module_init(struct weston_compositor *ec,
> return -1;
>
> test->compositor = ec;
> - weston_layer_init(&test->layer, &ec->cursor_layer.link);
> + weston_layer_init(ec, &test->layer);
> + weston_layer_set_position(&test->layer, WESTON_LAYER_POSITION_NORMAL);
Looks like this was intended to go immediately below the cursor layer,
so on top of everything else. Since it's a special layer for tests
only, how about using WESTON_LAYER_POSITION_CURSOR - 1?
>
> if (wl_global_create(ec->wl_display, &weston_test_interface, 1,
> test, bind_test) == NULL)
Very good work indeed.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161205/302e1a45/attachment-0001.sig>
More information about the wayland-devel
mailing list