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

[Pekka Paalanen]

On Wed, 22 Jun 2016 12:16:31 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> On 21.06.2016 14:05, Pekka Paalanen wrote:
> > 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.
> >   
> 
> Ugh, that's a valid concern.
> 
> > 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.
> >   
> 
> I wanted to split the shells stuff mainly because different people work(ed)
> on different shells. I like the first approach.

Hi,

ok, that is the way I would consider more proper too.

> >> ---
> >>  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.
> >   
> 
> I'm not sure I'm getting all this. We can discuss about it more on
> IRC or I can just copy/paste most of the text above :).

> >> @@ -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.
> >   
> 
> Again, I was trying to mimic the previous behavior.
> weston_view_update_transform assigns an output to the view, and when
> the output got assigned it was assumed the view was mapped.

Mimicing the old behaviour would require you to make
weston_surface_assing_output() set the surface->is_mapped based on the
various view->is_mapped. You don't do that, and I don't think we should
either because of what I explained above.

That makes us change the behaviour more than strictly necessary, but I
think it's worth it.


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/20160622/862a3104/attachment.sig>