RE: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
Hi Tanibata-san,
Tel. +49 5121 49 6937
> -Original Message-
> From: wayland-devel [mailto:wayland-devel-
> boun...@lists.freedesktop.org] On Behalf Of Tanibata, Nobuhiko
> (ADITJ/SWG)
> Sent: Montag, 17. August 2015 08:01
> To: Pekka Paalanen; Nobuhiko Tanibata
> Cc: Ishikawa, Tetsuri (ADITJ/SWG); securitych...@denso.co.jp; wayland-
> de...@lists.freedesktop.org
> Subject: RE: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not
> removed from list of ivi_layer when the ivi_surface is removed from the
> compositor.
>
>
>
> > -Original Message-
> > From: wayland-devel
> > [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of
> > Pekka Paalanen
> > Sent: Thursday, August 13, 2015 10:21 PM
> > To: Nobuhiko Tanibata
> > Cc: securitych...@denso.co.jp; wayland-devel@lists.freedesktop.org
> > Subject: Re: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is
> > not removed from list of ivi_layer when the ivi_surface is removed
> > from the compositor.
> >
> > On Fri, 7 Aug 2015 09:47:02 +0900
> > Nobuhiko Tanibata wrote:
> >
> > > The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface
> > > from a list of ivi_layer. In previous code, there is no trigger to
> > > refresh order of list, removing the ivi_surface, in commit_layer_list.
> > >
> > > To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to
> > > trigger refresh list of surface in commit_layer_list.
> > >
> > > In commit_layer_list, this patch also removes duplicated code in two
> > > conditions for IVI_NOTIFICATION_ADD/REMOVE.
> > >
> > > Signed-off-by: Nobuhiko Tanibata
> > >
> > > ---
> > > v2 changes:
> > > - fix 8 spaces to tab.
> > > - clean up duplicate code in commit_layer_list.
> > > - improve commit message.
> > >
> > > ivi-shell/ivi-layout.c | 28 +---
> > > 1 file changed, 9 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index
> > > 2974bb7..1b45003 100644
> > > --- a/ivi-shell/ivi-layout.c
> > > +++ b/ivi-shell/ivi-layout.c
> > > @@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout)
> > > if (!(ivilayer->event_mask &
> > > (IVI_NOTIFICATION_ADD |
> > IVI_NOTIFICATION_REMOVE)) ) {
> > > continue;
> >
> > Hi Tanibata-san,
> >
> > using 'continue', there is no need to have an else-branch. This would
> > save one level of indent in the remaining code.
> >
> > > - }
> > > -
> > > - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
> > > - wl_list_for_each_safe(ivisurf, next,
> > > - &ivilayer->order.surface_list,
> > order.link) {
> > > -
> > remove_ordersurface_from_layer(ivisurf);
> > > -
> > > - if
> > (!wl_list_empty(&ivisurf->order.link)) {
> > > -
> > wl_list_remove(&ivisurf->order.link);
> > > - }
> > > -
> > > - wl_list_init(&ivisurf->order.link);
> > > - ivisurf->event_mask |=
> > IVI_NOTIFICATION_REMOVE;
> >
> > You are removing this setting of the flag IVI_NOTIFICATION_REMOVE, but
> > in the code that remains I do not see that happening anymore in
> > commit_layer_list().
> >
> > However, I see it being set by ivi_layout_layer_remove_surface(). At
> > which time should the flag be set? Should the ADD flag not work the same
> way?
> >
> > Would it be essential to flag ivi-surfaces that get removed from
> > ivilayer->order.surface_list with IVI_NOTIFICATION_REMOVE, and
> > ivilayer->surfaces
> > that are added to flag with IVI_NOTIFICATION_ADD, and all remaining
> > surfaces even if they are reordered not be flagged at all?
> >
> > Or is it intended that all surfaces that are originally in the
> > ivilayer->order.surface_list are flagged as REMOVE, and all surfaces
> > ivilayer->in
> > the pending list are flagged as ADD? That would imply that surfaces
> > that were and still remain in the order list are flagged as both REMOVE and
> ADD.
> >
> > > - }
> > > -
> > > - wl_list_init(&ivilayer->order.surface_list);
> > > - }
> >
RE: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
> -Original Message-
> From: wayland-devel
> [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of Pekka
> Paalanen
> Sent: Thursday, August 13, 2015 10:21 PM
> To: Nobuhiko Tanibata
> Cc: securitych...@denso.co.jp; wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not
> removed from list of ivi_layer when the ivi_surface is removed from the
> compositor.
>
> On Fri, 7 Aug 2015 09:47:02 +0900
> Nobuhiko Tanibata wrote:
>
> > The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface
> > from a list of ivi_layer. In previous code, there is no trigger to
> > refresh order of list, removing the ivi_surface, in commit_layer_list.
> >
> > To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to
> > trigger refresh list of surface in commit_layer_list.
> >
> > In commit_layer_list, this patch also removes duplicated code in two
> > conditions for IVI_NOTIFICATION_ADD/REMOVE.
> >
> > Signed-off-by: Nobuhiko Tanibata
> > ---
> > v2 changes:
> > - fix 8 spaces to tab.
> > - clean up duplicate code in commit_layer_list.
> > - improve commit message.
> >
> > ivi-shell/ivi-layout.c | 28 +---
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index
> > 2974bb7..1b45003 100644
> > --- a/ivi-shell/ivi-layout.c
> > +++ b/ivi-shell/ivi-layout.c
> > @@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout)
> > if (!(ivilayer->event_mask &
> > (IVI_NOTIFICATION_ADD |
> IVI_NOTIFICATION_REMOVE)) ) {
> > continue;
>
> Hi Tanibata-san,
>
> using 'continue', there is no need to have an else-branch. This would save
> one level of indent in the remaining code.
>
> > - }
> > -
> > - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
> > - wl_list_for_each_safe(ivisurf, next,
> > - &ivilayer->order.surface_list,
> order.link) {
> > -
> remove_ordersurface_from_layer(ivisurf);
> > -
> > - if
> (!wl_list_empty(&ivisurf->order.link)) {
> > -
> wl_list_remove(&ivisurf->order.link);
> > - }
> > -
> > - wl_list_init(&ivisurf->order.link);
> > - ivisurf->event_mask |=
> IVI_NOTIFICATION_REMOVE;
>
> You are removing this setting of the flag IVI_NOTIFICATION_REMOVE, but in
> the code that remains I do not see that happening anymore in
> commit_layer_list().
>
> However, I see it being set by ivi_layout_layer_remove_surface(). At which
> time should the flag be set? Should the ADD flag not work the same way?
>
> Would it be essential to flag ivi-surfaces that get removed from
> ivilayer->order.surface_list with IVI_NOTIFICATION_REMOVE, and surfaces
> that are added to flag with IVI_NOTIFICATION_ADD, and all remaining surfaces
> even if they are reordered not be flagged at all?
>
> Or is it intended that all surfaces that are originally in the
> ivilayer->order.surface_list are flagged as REMOVE, and all surfaces in
> the pending list are flagged as ADD? That would imply that surfaces that
> were and still remain in the order list are flagged as both REMOVE and ADD.
>
> > - }
> > -
> > - wl_list_init(&ivilayer->order.surface_list);
> > - }
> > -
> > - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
> > + } else {
> > wl_list_for_each_safe(ivisurf, next,
> >
> &ivilayer->order.surface_list, order.link) {
> >
> remove_ordersurface_from_layer(ivisurf);
> > @@ -843,6 +825,13 @@ commit_layer_list(struct ivi_layout *layout)
> > }
> >
> > wl_list_init(&ivilayer->order.surface_list);
> > +
> > + /**
> > +* Following ivilayer->pending.surface_list must
> be maintained by
> > +* a function who will set these masks. Order of
> surfaces in a
> > +* layer is restructured here. if there is no
> surface in
> > +* surface_list, the following code is skipped.
> > +*/
> > wl_list_for_each(ivisurf,
> &ivilayer->pending.surface_list,
Re: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
On Fri, 7 Aug 2015 09:47:02 +0900
Nobuhiko Tanibata wrote:
> The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface
> from a list of ivi_layer. In previous code, there is no trigger to
> refresh order of list, removing the ivi_surface, in commit_layer_list.
>
> To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to trigger
> refresh list of surface in commit_layer_list.
>
> In commit_layer_list, this patch also removes duplicated code in two
> conditions for IVI_NOTIFICATION_ADD/REMOVE.
>
> Signed-off-by: Nobuhiko Tanibata
> ---
> v2 changes:
> - fix 8 spaces to tab.
> - clean up duplicate code in commit_layer_list.
> - improve commit message.
>
> ivi-shell/ivi-layout.c | 28 +---
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> index 2974bb7..1b45003 100644
> --- a/ivi-shell/ivi-layout.c
> +++ b/ivi-shell/ivi-layout.c
> @@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout)
> if (!(ivilayer->event_mask &
> (IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE)) ) {
> continue;
Hi Tanibata-san,
using 'continue', there is no need to have an else-branch. This would
save one level of indent in the remaining code.
> - }
> -
> - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
> - wl_list_for_each_safe(ivisurf, next,
> - &ivilayer->order.surface_list, order.link) {
> - remove_ordersurface_from_layer(ivisurf);
> -
> - if (!wl_list_empty(&ivisurf->order.link)) {
> - wl_list_remove(&ivisurf->order.link);
> - }
> -
> - wl_list_init(&ivisurf->order.link);
> - ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE;
You are removing this setting of the flag IVI_NOTIFICATION_REMOVE, but
in the code that remains I do not see that happening anymore in
commit_layer_list().
However, I see it being set by ivi_layout_layer_remove_surface(). At
which time should the flag be set? Should the ADD flag not work the
same way?
Would it be essential to flag ivi-surfaces that get removed from
ivilayer->order.surface_list with IVI_NOTIFICATION_REMOVE, and surfaces
that are added to flag with IVI_NOTIFICATION_ADD, and all remaining
surfaces even if they are reordered not be flagged at all?
Or is it intended that all surfaces that are originally in the
ivilayer->order.surface_list are flagged as REMOVE, and all surfaces in
the pending list are flagged as ADD? That would imply that surfaces
that were and still remain in the order list are flagged as both REMOVE
and ADD.
> - }
> -
> - wl_list_init(&ivilayer->order.surface_list);
> - }
> -
> - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
> + } else {
> wl_list_for_each_safe(ivisurf, next,
> &ivilayer->order.surface_list,
> order.link) {
> remove_ordersurface_from_layer(ivisurf);
> @@ -843,6 +825,13 @@ commit_layer_list(struct ivi_layout *layout)
> }
>
> wl_list_init(&ivilayer->order.surface_list);
> +
> + /**
> + * Following ivilayer->pending.surface_list must be
> maintained by
> + * a function who will set these masks. Order of
> surfaces in a
> + * layer is restructured here. if there is no surface in
> + * surface_list, the following code is skipped.
> + */
> wl_list_for_each(ivisurf,
> &ivilayer->pending.surface_list,
>pending.link) {
> if(!wl_list_empty(&ivisurf->order.link)){
> @@ -2565,6 +2554,7 @@ ivi_layout_layer_remove_surface(struct ivi_layout_layer
> *ivilayer,
> }
>
> remsurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> + ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> }
>
> static int32_t
All the flagging issues aside, I see what this patch does.
Previously removing a ivi-surface didn't work at all. With this patch,
whether ivi-surfaces are added or removed from the ivi-layer, the code
will always completely reconstruct the ivilayer->order.surface_list
from the pending list. In that sense:
Reviewed-by: Pekka Paalanen
Do you want me to push this patch as is?
Thanks,
pq
pgp3VYI0xPmki.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface
from a list of ivi_layer. In previous code, there is no trigger to
refresh order of list, removing the ivi_surface, in commit_layer_list.
To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to trigger
refresh list of surface in commit_layer_list.
In commit_layer_list, this patch also removes duplicated code in two
conditions for IVI_NOTIFICATION_ADD/REMOVE.
Signed-off-by: Nobuhiko Tanibata
---
v2 changes:
- fix 8 spaces to tab.
- clean up duplicate code in commit_layer_list.
- improve commit message.
ivi-shell/ivi-layout.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
index 2974bb7..1b45003 100644
--- a/ivi-shell/ivi-layout.c
+++ b/ivi-shell/ivi-layout.c
@@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout)
if (!(ivilayer->event_mask &
(IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE)) ) {
continue;
- }
-
- if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
- wl_list_for_each_safe(ivisurf, next,
- &ivilayer->order.surface_list, order.link) {
- remove_ordersurface_from_layer(ivisurf);
-
- if (!wl_list_empty(&ivisurf->order.link)) {
- wl_list_remove(&ivisurf->order.link);
- }
-
- wl_list_init(&ivisurf->order.link);
- ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE;
- }
-
- wl_list_init(&ivilayer->order.surface_list);
- }
-
- if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
+ } else {
wl_list_for_each_safe(ivisurf, next,
&ivilayer->order.surface_list,
order.link) {
remove_ordersurface_from_layer(ivisurf);
@@ -843,6 +825,13 @@ commit_layer_list(struct ivi_layout *layout)
}
wl_list_init(&ivilayer->order.surface_list);
+
+ /**
+* Following ivilayer->pending.surface_list must be
maintained by
+* a function who will set these masks. Order of
surfaces in a
+* layer is restructured here. if there is no surface in
+* surface_list, the following code is skipped.
+*/
wl_list_for_each(ivisurf,
&ivilayer->pending.surface_list,
pending.link) {
if(!wl_list_empty(&ivisurf->order.link)){
@@ -2565,6 +2554,7 @@ ivi_layout_layer_remove_surface(struct ivi_layout_layer
*ivilayer,
}
remsurf->event_mask |= IVI_NOTIFICATION_REMOVE;
+ ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
}
static int32_t
--
1.8.3.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
