On Thu, 23 Jun 2016 14:41:57 +0200 Giulio Camuffo <[email protected]> wrote:
> 2016-06-18 19:15 GMT+02:00 Armin Krezović <[email protected]>: > > 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ć <[email protected]> > > --- > > 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
pgpCCfqqRDN4x.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
