On Sat, 18 Jun 2016 19:15:23 +0200 Armin Krezović <krezovic.ar...@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.ar...@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
pgpmbyee_lXjz.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel