On 05/12/2016 17:00, Pekka Paalanen wrote:
On Mon, 5 Dec 2016 16:48:38 +0100
Quentin Glidic <[email protected]> wrote:
On 05/12/2016 16:29, Pekka Paalanen wrote:
On Mon, 11 Jul 2016 11:29:40 +0200
Quentin Glidic <[email protected]> wrote:
From: Quentin Glidic <[email protected]>
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 <[email protected]>
Acked-by: Pekka Paalanen <[email protected]>
---
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
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
