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

[Pekka Paalanen]

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?


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: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150813/ef931265/attachment-0001.sig>