[PATCH weston 1/2] desktop-shell: fix output removal for background/panel

[Pekka Paalanen]

On Tue, 26 Jun 2018 09:28:42 +0000
Marius-cristian Vlad <marius-cristian.vlad at nxp.com> wrote:

> On Thu, 2018-06-21 at 15:53 +0300, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > When the compositor has multiple outputs (not clones) and one of them
> > is
> > removed, the ones remaining to the right will be moved to close the
> > gap.
> > Because reflowing the remaining outputs happens before removing the
> > wl_output global, we get the new output x,y before the removal. This
> > causes us to consider the remaining output immediately to the right
> > of
> > the removed output to be a clone of the removed output whose x,y
> > don't
> > get updated. That will then hit the two assertions this patch
> > removes.
> > 
> > The reason the assertions were not actually hit is because of a
> > compositor bug which moved the remaining outputs in the wrong
> > direction.
> > The next patch will fix the reflow, so we need this patch first to
> > avoid
> > the asserts.
> > 
> > Remove the assertions and hand over the background and panel if the
> > "clone" does not already have them. If the clone already has them, we
> > destroy the unnecessary background and panel.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  clients/desktop-shell.c | 37 ++++++++++++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 13 deletions(-)
> > 
> > diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> > index 6d19d029..fcc0b657 100644
> > --- a/clients/desktop-shell.c
> > +++ b/clients/desktop-shell.c
> > @@ -1337,19 +1337,30 @@ output_remove(struct desktop *desktop, struct
> > output *output)
> >  	}
> >  
> >  	if (rep) {
> > -		/* If found, hand over the background and panel so
> > they don't
> > -		 * get destroyed. */
> > -		assert(!rep->background);
> > -		assert(!rep->panel);
> > -
> > -		rep->background = output->background;
> > -		output->background = NULL;
> > -		rep->background->owner = rep;
> > -
> > -		rep->panel = output->panel;
> > -		output->panel = NULL;
> > -		if (rep->panel)
> > -			rep->panel->owner = rep;
> > +		/* If found and it does not already have a
> > background or panel,
> > +		 * hand over the background and panel so they don't
> > get
> > +		 * destroyed.
> > +		 *
> > +		 * We never create multiple backgrounds or panels
> > for clones,
> > +		 * but if the compositor moves outputs, a pair of
> > wl_outputs
> > +		 * might become "clones". This may happen
> > temporarily when
> > +		 * an output is about to be removed and the rest are
> > reflowed.
> > +		 * In this case it is correct to let the
> > background/panel be
> > +		 * destroyed.
> > +		 */
> > +
> > +		if (!rep->background) {
> > +			rep->background = output->background;
> > +			output->background = NULL;
> > +			rep->background->owner = rep;
> > +		}
> > +
> > +		if (!rep->panel) {
> > +			rep->panel = output->panel;
> > +			output->panel = NULL;
> > +			if (rep->panel)  
> Guess copy-pasta from above, but is the test necessary? 

A background is mandatory, but a panel is optional: output->panel can
be NULL. Therefore the test is not needed for the background but it is
needed for the panel. This is the same as in the old code.

> 
> Reviewed-by: Marius Vlad <marius-cristian.vlad at nxp.com>

Thanks,
pq


> 
> > +				rep->panel->owner = rep;
> > +		}
> >  	}
> >  
> >  	output_destroy(output)  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180627/09787411/attachment.sig>