[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.

[Tanibata, Nobuhiko (ADITJ/SWG)]

> -----Original Message-----
> From: wayland-devel
> [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf Of Pekka
> Paalanen
> Sent: Thursday, August 13, 2015 10:21 PM
> To: Nobuhiko Tanibata
> Cc: securitycheck at denso.co.jp; wayland-devel at 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 <nobuhiko_tanibata at xddp.denso.co.jp> 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 <nobuhiko_tanibata at xddp.denso.co.jp>
> > ---
> > 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 <pekka.paalanen at collabora.co.uk>
> 
> Do you want me to push this patch as is?
[ntanibata] Hi Pekka-san,
Thank you for review. No, I don't. I shall reconsider name of masks: IVI_NOTIFICATION_REMOVE/ADD.

BR,
Nobuhiko Tanibata
> 
> 
> Thanks,
> pq