[PATCH weston v3] libweston: Position layers in an absolute way
Pekka Paalanen
ppaalanen at gmail.com
Mon Dec 5 16:00:33 UTC 2016
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.
> >> +}
> >> +
> >> +/** 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.
> >> + */
> >> +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?
> >> 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.
> >>
> >> if (wl_global_create(ec->wl_display, &weston_test_interface, 1,
> >> test, bind_test) == NULL)
> >
> > Very good work indeed.
>
> Thanks (for the review too)!
>
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/85ba5cc4/attachment.sig>
More information about the wayland-devel
mailing list