Re: [PATCH weston 1/2] desktop-shell: fix output removal for background/panel
On Tue, 26 Jun 2018 09:28:42 +
Marius-cristian Vlad wrote:
> On Thu, 2018-06-21 at 15:53 +0300, Pekka Paalanen wrote:
> > From: Pekka Paalanen
> >
> > 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
> > ---
> > 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
Thanks,
pq
>
> > + rep->panel->owner = rep;
> > + }
> > }
> >
> > output_destroy(output)
pgppGKQshxttW.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] desktop-shell: fix output removal for background/panel
On Thu, 2018-06-21 at 15:53 +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen
>
> 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
> ---
> 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?
Reviewed-by: Marius Vlad
> + rep->panel->owner = rep;
> + }
> }
>
> output_destroy(output);
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 1/2] desktop-shell: fix output removal for background/panel
From: Pekka Paalanen
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
---
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)
+ rep->panel->owner = rep;
+ }
}
output_destroy(output);
--
2.16.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
