[PATCH weston v3] libweston: Position layers in an absolute way
Quentin Glidic
sardemff7+wayland at sardemff7.net
Mon Dec 5 15:48:38 UTC 2016
On 05/12/2016 16:29, Pekka Paalanen wrote:
> 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.
I think I overlooked all the changes for libweston-desktop, or some
other patch that got applied since then.
>> 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?
Honestly I cannot remember why I used .next here… I vaguely remember
something like layers are not ordered as one would think, but that’s
all. I will try to re-figure it out.
>> +}
>> +
>> +/** 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?
I think set_position and unset_position should always be safe. That
would make then easier to use.
Maybe Giulio could help us on this choice?
>> + */
>> +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.
I use the enum everywhere for documentation/consistency, but I don’t
mind either way.
>> 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?
Well, ordering was dependent of loading order. I am not sure it was
intentional to have this layer at this position, since there is no
comment saying “since we load after the shell, we are sure to have our
layer on top”. Your call. :-)
>>
>> if (wl_global_create(ec->wl_display, &weston_test_interface, 1,
>> test, bind_test) == NULL)
>
> Very good work indeed.
Thanks (for the review too)!
--
Quentin “Sardem FF7” Glidic
More information about the wayland-devel
mailing list