[PATCH weston 08/11] compositor: Untangle mapedness from outputs

[Pekka Paalanen]

On Thu, 23 Jun 2016 14:41:57 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 2016-06-18 19:15 GMT+02:00 Armin Krezović <krezovic.armin at gmail.com>:
> > 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>
> > ---
> >  src/compositor.c  | 29 ++++++++++-------------------
> >  src/compositor.h  |  4 ++++
> >  src/data-device.c |  2 ++
> >  src/input.c       |  2 ++
> >  4 files changed, 18 insertions(+), 19 deletions(-)
> >
> > 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;
> >  }  
> 
> I don't understand what this function means. It's here from before the
> surface/view split happened, but right now i am not sure what the
> purpose of it is, and i would argue it shuold go. How can a surface be
> mapped if no views of it are mapped? Maybe you have a different
> definition of "mapped" than me, in which case i would like to see it
> as part of the documentation for these functions.
> For the use-case Pekka mentioned, remembering if a surface was mapped
> earlier and so retaining information like the position, all that can
> be done by having custom logic in the shell_surface, which knows all
> it needs to know about the surface and the views. We don't need the
> surface mappedness for that.

It can be done, yes, for shell surfaces.

You can check all the calls to weston_surface_is_mapped() with 'git
grep -p weston_surface_is_mapped'. Ignoring all shells, we are left
with the following notable places:

weston_view_unmap():

	Skips checking and fixing input device foci if the surface
	remains mapped after the view is unmapped. This could be
	regarded as a bug though, at least for the foci that are
	tracked by the view rather than the surface.


weston_surface_attach(), called from weston_surface_commit_state():

	Avoids a redundant call to weston_surface_unmap().


view_list_add_subsurface_view(), part of
weston_compositor_build_view_list() machinery:

	Only if surface is mapped, creates a new view for it
	dynamically, as necessary, or re-uses a stashed view for it.
	This is crucial to be done only for mapped surfaces, because
	there are conditions that prevent a sub-surface from being
	mapped even if it has both the role and the content. This can
	also be the first time a view is being created, so there may
	not be a view to check for. Furthermore, sub-surface views are
	to be created for *each* parent surface view. If your shell
	makes multiple views for the parent, the sub-surfaces will come
	along automatically.

There are also other places, but those are either avoiding redundant
calls to weston_surface_unmap() or protecting the layer setup, i.e.
mapping.

You're right, weston_surface_is_mapped() looks like it could go. Each
shell needs to track it's own surface mappedness state, struct
weston_subsurface needs to add that too, weston_surface_unmap() can
probably cope with redundant calls, and weston_view_unmap()'s focus
handling would need fixing anyway.

Changing weston_surface_is_mapped() to look at view assignments to
layerr would not work for sub-surfaces.

IOW, it's not in scope for Armin's project, I don't think.

The immediate goal is to get outputs separated mappedness, so that
Armin can continue his project on outputs.

> 
> >
> >  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);
> > @@ -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;
> >  }
> >
> > @@ -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;
> >
> >         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;  
> 
> If you always set is_mapped to true here, what is the difference to
> just checking if the view is in the view_list?

Subsurfaces get *all* their views dynamically created and destroyed as
needed, so determining whether a sub-surface is mapped yet (should be
drawn or not, e.g. has the parent seen a commit yet etc.) there may not
be views to check.

Did you know anything about the sloppy focus handling with
weston_surface_unmap() and weston_view_unmap(), is it intentional or an
oversight? Does it look equally wrong to you as it does to me?

Do you think dropping "mappedness inference" from
weston_surface_assign_output() path already in this patch causes
problems, or should we just replicate the is_mapped inference logic
from the output assignment logic?


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/20160627/c347bce6/attachment.sig>