[PATCH weston] ivi-shell: bugfix, list of layers on a screen are cumulated when set render order is called several time in one commitchanges.

[Pekka Paalanen]

On Wed, 19 Aug 2015 11:25:03 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:

> - Always clear pending list at set_render_order API.
> - Introduce the dirty parameter for triggering the render order change.
> - IVI_NOTIFICATION_REMOVE/ADD flags are set only at commit_screen_list.

Hi,

the same comments about the commit message and summary as earlier.

Essentially all my comments here are just references to the comments I
made in
http://lists.freedesktop.org/archives/wayland-devel/2015-August/023987.html .

> 
> Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> ---
>  ivi-shell/ivi-layout.c |   45 +++++++++++----------------------------------
>  1 file changed, 11 insertions(+), 34 deletions(-)
> 
> diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> index bc8aead..898a901 100644
> --- a/ivi-shell/ivi-layout.c
> +++ b/ivi-shell/ivi-layout.c
> @@ -93,14 +93,13 @@ struct ivi_layout_screen {
>  	struct ivi_layout *layout;
>  	struct weston_output *output;
>  
> -	uint32_t event_mask;
> -
>  	struct {
>  		struct wl_list layer_list;
>  		struct wl_list link;
>  	} pending;
>  
>  	struct {
> +		int dirty;
>  		struct wl_list layer_list;
>  		struct wl_list link;
>  	} order;
> @@ -431,7 +430,6 @@ create_screen(struct weston_compositor *ec)
>  		count++;
>  
>  		iviscrn->output = output;
> -		iviscrn->event_mask = 0;
>  
>  		wl_list_init(&iviscrn->pending.layer_list);
>  		wl_list_init(&iviscrn->pending.link);
> @@ -853,7 +851,7 @@ commit_screen_list(struct ivi_layout *layout)
>  	struct ivi_layout_surface *ivisurf  = NULL;
>  
>  	wl_list_for_each(iviscrn, &layout->screen_list, link) {
> -		if (iviscrn->event_mask & IVI_NOTIFICATION_REMOVE) {
> +		if (iviscrn->order.dirty) {
>  			wl_list_for_each_safe(ivilayer, next,
>  					      &iviscrn->order.layer_list, order.link) {
>  				remove_orderlayer_from_screen(ivilayer);
> @@ -865,21 +863,9 @@ commit_screen_list(struct ivi_layout *layout)
>  				wl_list_init(&ivilayer->order.link);
>  				ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
>  			}
> -		}
> -
> -		if (iviscrn->event_mask & IVI_NOTIFICATION_ADD) {
> -			wl_list_for_each_safe(ivilayer, next,
> -					      &iviscrn->order.layer_list, order.link) {
> -				remove_orderlayer_from_screen(ivilayer);
> -
> -				if (!wl_list_empty(&ivilayer->order.link)) {
> -					wl_list_remove(&ivilayer->order.link);
> -				}
> -
> -				wl_list_init(&ivilayer->order.link);
> -			}
>  
>  			wl_list_init(&iviscrn->order.layer_list);

assert(wl_list_empty(&iviscrn->order.layer_list));

> +
>  			wl_list_for_each(ivilayer, &iviscrn->pending.layer_list,
>  					 pending.link) {
>  				wl_list_insert(&iviscrn->order.layer_list,
> @@ -887,9 +873,9 @@ commit_screen_list(struct ivi_layout *layout)
>  				add_orderlayer_to_screen(ivilayer, iviscrn);
>  				ivilayer->event_mask |= IVI_NOTIFICATION_ADD;
>  			}
> -		}
>  
> -		iviscrn->event_mask = 0;
> +			iviscrn->order.dirty = 0;
> +		}
>  
>  		/* Clear view list of layout ivi_layer */
>  		wl_list_init(&layout->layout_layer.view_list.link);

(Btw. as a side comment here, this init is dangerous. If this link was a
part of any list, that list is now broken. If the code happens to do a
wl_list_remove() on a link that was adjacent to this link before the
init, then this link will break too. It's not obvious (at least to me)
that the link cannot be in any list or that all members of the list are
reset the same way, including the head.)

> @@ -2330,7 +2316,7 @@ ivi_layout_screen_add_layer(struct ivi_layout_screen *iviscrn,
>  		}
>  	}
>  
> -	iviscrn->event_mask |= IVI_NOTIFICATION_ADD;
> +	iviscrn->order.dirty = 1;
>  
>  	return IVI_SUCCEEDED;
>  }
> @@ -2353,24 +2339,15 @@ ivi_layout_screen_set_render_order(struct ivi_layout_screen *iviscrn,
>  
>  	wl_list_for_each_safe(ivilayer, next,
>  			      &iviscrn->pending.layer_list, pending.link) {
> +		if (!wl_list_empty(&ivilayer->pending.link)) {

This is a bogus check, as explained in my other review.

> +			wl_list_remove(&ivilayer->pending.link);
> +		}
> +
>  		wl_list_init(&ivilayer->pending.link);
>  	}
>  
>  	wl_list_init(&iviscrn->pending.layer_list);

assert(wl_list_empty(&iviscrn->pending.layer_list));

>  
> -	if (pLayer == NULL) {
> -		wl_list_for_each_safe(ivilayer, next, &iviscrn->pending.layer_list, pending.link) {
> -			if (!wl_list_empty(&ivilayer->pending.link)) {
> -				wl_list_remove(&ivilayer->pending.link);
> -			}
> -
> -			wl_list_init(&ivilayer->pending.link);
> -		}
> -
> -		iviscrn->event_mask |= IVI_NOTIFICATION_REMOVE;
> -		return IVI_SUCCEEDED;
> -	}
> -
>  	for (i = 0; i < number; i++) {
>  		id_layer = &pLayer[i]->id_layer;
>  		wl_list_for_each(ivilayer, &layout->layer_list, link) {
> @@ -2388,7 +2365,7 @@ ivi_layout_screen_set_render_order(struct ivi_layout_screen *iviscrn,
>  		}
>  	}
>  
> -	iviscrn->event_mask |= IVI_NOTIFICATION_ADD;
> +	iviscrn->order.dirty = 1;
>  
>  	return IVI_SUCCEEDED;
>  }

Essentially a good patch, needs a little work on few details and the
commit message.


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/20150820/82472059/attachment.sig>