[PATCH weston v3] libweston: Position layers in an absolute way
Quentin Glidic
sardemff7+wayland at sardemff7.net
Sat Dec 17 13:05:53 UTC 2016
On 05/12/2016 17:00, Pekka Paalanen wrote:
> On Mon, 5 Dec 2016 16:48:38 +0100
> Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:
>
>> 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.
>
> I believe the workspace code has been there for... years? :-)
>
> struct workspace embeds a struct weston_layer.
Sure, then I didn’t touch it because I didn’t break it. It’s now done in v5.
>>>> +}
>>>> +
>>>> +/** 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?
>
> FWIW, I would favour being safe than forbid in this case.
Nice, done in v5.
>>>> + */
>>>> +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.
>
> I'm not sure how strict the C enum is conceptually. If someone did
> switch(enum), the compiler can check if you checked every value and
> then you don't need the default target, IIRC.
>
> OTOH, having it an enum allows a debugger to show the symbolic value.
> Maybe that's more useful here?
I’ll keep the enum for now, let’s see if anyone has a strong opinion here.
>>>> 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. :-)
>
> Ok, let's define it like I said then. I think the only tests that
> actually care about the layer are screenshot-based tests and maybe some
> input test(?), but I'm fairly sure they expect the test surface to be
> top-most.
Done.
v5 is sent.
Cheers,
>>>>
>>>> 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