[PATCH weston 08/11] compositor: Untangle mapedness from outputs
Pekka Paalanen
ppaalanen at gmail.com
Tue Jun 21 12:05:13 UTC 2016
On Sat, 18 Jun 2016 19:15:23 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:
> Currently, weston assumes a surface/view is mapped if
> it has an output assigned. In a zero outputs scenario,
> this isn't really desirable.
>
> This patch introduces a new flag to weston_surface and
> weston_view, which has to be set manually to indicate
> that a surface/view is mapped.
>
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
Hi Armin,
there is a slight problem with the split and ordering in patches 8 -
11. Patch 8 removes the old conditions, which breaks "everything" and
the follow-up patches fix things up piece by piece. The problem is that
we now have known-broken commits from the start. Those will make
bisecting unrelated issues harder.
There are two possible solutions:
- split patch 8 to to only add the new fields, and make a new patch as
the final one in the series that switches the
weston_{view,surface}_is_mapped() to use the new flags
- squash all patches 8 - 11 into one
Because the changes in patches 9 - 11 are fairly trivial and unlikely
to break anything in themselves, the patch that switches the
*_is_mapped() functions will be the one causing the regressions if any.
Hence it would be also ok to just squash these into one patch. It
doesn't make too much difference for reviews, and no difference for
bisecting.
> ---
> src/compositor.c | 29 ++++++++++-------------------
> src/compositor.h | 4 ++++
> src/data-device.c | 2 ++
> src/input.c | 2 ++
> 4 files changed, 18 insertions(+), 19 deletions(-)
There is also a more subtle change of semantics in this patch, which
would be good to underline in the commit message.
Previously when mappedness was determined by outputs, unmapping the
last view unmapped also the surface, or so I believe. That no longer
happens, which is actually a good thing. This allows a shell to unmap
all views without losing the mappedness information of the surface.
A practical example of this is hiding a desktop-shell top-level window.
When the window is shown again, it should appear on the position where
it was hidden, but that will not happen if desktop-shell thinks the
surface is not mapped. Instead, desktop-shell will randomize the window
position again. I haven't looked if this actually happens or if there
is code to work around that, but I think the change by this patch is
for the better.
The mappedness states of a surface and its views are no longer
implicitly connected.
>
> diff --git a/src/compositor.c b/src/compositor.c
> index d246046..b25c671 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -1549,19 +1549,13 @@ weston_view_set_mask_infinite(struct weston_view *view)
> WL_EXPORT bool
> weston_view_is_mapped(struct weston_view *view)
> {
> - if (view->output)
> - return true;
> - else
> - return false;
> + return view->is_mapped;
> }
>
> WL_EXPORT bool
> weston_surface_is_mapped(struct weston_surface *surface)
> {
> - if (surface->output)
> - return true;
> - else
> - return false;
> + return surface->is_mapped;
> }
>
> static void
> @@ -1738,6 +1732,7 @@ weston_view_unmap(struct weston_view *view)
> weston_view_damage_below(view);
> view->output = NULL;
> view->plane = NULL;
> + view->is_mapped = false;
> weston_layer_entry_remove(&view->layer_link);
> wl_list_remove(&view->link);
> wl_list_init(&view->link);
Looks like weston_view_unmap() has a problem. It won't fix up the input
device foci unless the surface is not mapped. So even before this
patch, if a shell unmaps a view that had pointer focus, the pointer
focus is not removed. The same for touch. But this does not apply to
kbd focus, because kbd focus is tracked by the surface, not by the view.
Then I wonder if the sloppy handling of pointer focus is actually
relied on somewhere, or if it's just accidental. Or both.
Giulio, do you have any recommendations on what to do here?
Since this ties in with the issue below...
Fixing this is not a matter for this patch, though.
> @@ -1769,6 +1764,7 @@ weston_surface_unmap(struct weston_surface *surface)
>
> wl_list_for_each(view, &surface->views, surface_link)
> weston_view_unmap(view);
> + surface->is_mapped = false;
> surface->output = NULL;
Because weston_view_unmap() is called first, and surface->is_mapped is
set only afterwards, the input foci clean-up never runs.
I think surface->is_mapped = false should be before the
weston_view_unmap(). That way at least weston_surface_unmap() will work
as expected. Previously, it relied on the last call to
weston_view_unmap() -> weston_surface_assign_output() to mark the
surface as not mapped via output assignment, so that the input foci
clean-up was executed once all views were unmapped.
> }
>
> @@ -2131,6 +2127,7 @@ view_list_add_subsurface_view(struct weston_compositor *compositor,
>
> view->parent_view = parent;
> weston_view_update_transform(view);
> + view->is_mapped = true;
This looks correct.
>
> if (wl_list_empty(&sub->surface->subsurface_list)) {
> wl_list_insert(compositor->view_list.prev, &view->link);
> @@ -2152,6 +2149,7 @@ view_list_add(struct weston_compositor *compositor,
> struct weston_subsurface *sub;
>
> weston_view_update_transform(view);
> + view->is_mapped = true;
This I am not so sure about. Isn't it the responsibility of whoever
puts a view on a layer to set is_mapped to true?
Doing it here might paper over some overlooked places.
>
> if (wl_list_empty(&view->surface->subsurface_list)) {
> wl_list_insert(compositor->view_list.prev, &view->link);
> @@ -3244,7 +3242,6 @@ subsurface_get_label(struct weston_surface *surface, char *buf, size_t len)
> static void
> subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
> {
> - struct weston_compositor *compositor = surface->compositor;
> struct weston_view *view;
>
> wl_list_for_each(view, &surface->views, surface_link)
> @@ -3256,8 +3253,9 @@ subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
> * mapped, parent is not in a visible layer, so this sub-surface
> * will not be drawn either.
> */
> - if (!weston_surface_is_mapped(surface)) {
> - struct weston_output *output;
> +
> + if (!weston_surface_is_mapped(surface))
> + surface->is_mapped = true;
>
> /* Cannot call weston_view_update_transform(),
> * because that would call it also for the parent surface,
> @@ -3265,18 +3263,11 @@ subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
> * inconsistent state, where the window could never be
> * mapped.
> *
> - * Instead just assign any output, to make
> + * Instead just force the is_mapped flag on, to make
> * weston_surface_is_mapped() return true, so that when the
> * parent surface does get mapped, this one will get
> * included, too. See view_list_add().
> */
> - assert(!wl_list_empty(&compositor->output_list));
> - output = container_of(compositor->output_list.next,
> - struct weston_output, link);
> -
> - surface->output = output;
> - weston_surface_update_output_mask(surface, 1u << output->id);
> - }
Correct, but I'd still like to keep the braces. The comment block looks
quite out of place otherwise.
> }
>
> static struct weston_subsurface *
> diff --git a/src/compositor.h b/src/compositor.h
> index 9e5155c..8770982 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -964,6 +964,8 @@ struct weston_view {
>
> /* Per-surface Presentation feedback flags, controlled by backend. */
> uint32_t psf_flags;
> +
> + bool is_mapped;
> };
>
> struct weston_surface_state {
> @@ -1082,6 +1084,8 @@ struct weston_surface {
> const char *role_name;
>
> struct weston_timeline_object timeline;
> +
> + bool is_mapped;
> };
>
> struct weston_subsurface {
> diff --git a/src/data-device.c b/src/data-device.c
> index f04f030..44a08f9 100644
> --- a/src/data-device.c
> +++ b/src/data-device.c
> @@ -421,6 +421,8 @@ drag_surface_configure(struct weston_drag *drag,
> weston_layer_entry_insert(list, &drag->icon->layer_link);
> weston_view_update_transform(drag->icon);
> pixman_region32_clear(&es->pending.input);
> + es->is_mapped = true;
> + drag->icon->is_mapped = true;
Looks correct.
> }
>
> drag->dx += sx;
> diff --git a/src/input.c b/src/input.c
> index 08378d1..5ccc75f 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -1950,6 +1950,8 @@ pointer_cursor_surface_configure(struct weston_surface *es,
> weston_layer_entry_insert(&es->compositor->cursor_layer.view_list,
> &pointer->sprite->layer_link);
> weston_view_update_transform(pointer->sprite);
> + es->is_mapped = true;
> + pointer->sprite->is_mapped = true;
> }
> }
>
Looks correct.
I didn't yet search for overlooked places, but I should do that once I
have reviewed the last patches of this series.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160621/e26bf144/attachment.sig>
More information about the wayland-devel
mailing list