[PATCH weston] ivi-shell: Damage view below after unmapping

[Pekka Paalanen]

On Tue, 17 Jan 2017 12:30:56 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:

> If ivilayer or ivisurf of ivi_view is made invisible in the
> commit_changes call, we have to damage the weston_view below this
> ivi_view. Otherwise content of this ivi_view will stay visible.

Hi Emre,

from a quick check, I think it is indeed correct to call
weston_view_damage_below() when removing the view from the active
scenegraph. weston_view_geometry_dirty() would not be effective,
because the view would never hit weston_view_update_transform() via
weston_output_repaint().

At first sight, the commit message seems to have very little to do with
the full rewrite of commit_changes(). Could you explain and justify the
rewrite in the commit message?

It makes
	assert(ivi_view->on_layer == ivilayer);
in update_prop() apparently useless, as we no longer traverse the
ivi-layer's list of ivi-views and so will not check for its consistency.

> Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> ---
>  ivi-shell/ivi-layout.c |   41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> index 60d05c4..cc01de3 100644
> --- a/ivi-shell/ivi-layout.c
> +++ b/ivi-shell/ivi-layout.c
> @@ -666,28 +666,35 @@ commit_changes(struct ivi_layout *layout)
>  {
>  	struct ivi_layout_screen  *iviscrn  = NULL;
>  	struct ivi_layout_layer   *ivilayer = NULL;
> +	struct ivi_layout_surface   *ivisurf = NULL;
>  	struct ivi_layout_view *ivi_view  = NULL;
>  
> -	wl_list_for_each(iviscrn, &layout->screen_list, link) {
> -		wl_list_for_each(ivilayer, &iviscrn->order.layer_list, order.link) {
> -			/*
> -			 * If ivilayer is invisible, weston_view of ivisurf doesn't
> -			 * need to be modified.
> -			 */
> -			if (ivilayer->prop.visibility == false)
> -				continue;
> +	wl_list_for_each(ivi_view, &layout->view_list, link) {
> +		ivilayer = ivi_view->on_layer;
> +		ivisurf = ivi_view->ivisurf;
> +		iviscrn = ivilayer->on_screen;

Does something guarantee that ivilayer cannot be NULL?

>  
> -			wl_list_for_each(ivi_view, &ivilayer->order.view_list, order_link) {
> -				/*
> -				 * If ivilayer is invisible, weston_view of ivisurf doesn't
> -				 * need to be modified.
> -				 */
> -				if (ivi_view->ivisurf->prop.visibility == false)
> -					continue;
> +		/*
> +		 * If ivi_view is not visible on a screen, weston_view of ivi_view
> +		 * doesn't need to be modified.
> +		 */
> +		if (!wl_list_length(&ivi_view->order_link) || !iviscrn)

Use wl_list_empty() instead of wl_list_length().

> +			continue;
>  
> -				update_prop(iviscrn, ivilayer, ivi_view);
> -			}
> +		/*
> +		 * If ivilayer or ivisurf of ivi_view is made invisible in this
> +		 * commit_changes call, we have to damage the weston_view below this
> +		 * ivi_view. Otherwise content of this ivi_view will stay visible.
> +		 */
> +		if ((!ivilayer->prop.visibility &&
> +			(ivilayer->prop.event_mask & IVI_NOTIFICATION_VISIBILITY)) ||
> +			(!ivisurf->prop.visibility &&
> +			(ivisurf->prop.event_mask & IVI_NOTIFICATION_VISIBILITY))) {
> +			weston_view_damage_below(ivi_view->view);
> +			continue;

Does this now skip update_prop() for things made invisible in this
cycle, while previously they went through update_prop()?
I think the commit message should explain why this change.

>  		}
> +
> +		update_prop(iviscrn, ivilayer, ivi_view);
>  	}
>  }
>  

While I think the idea is good, the patch seems to be doing too much in
one go and without justification. But, I have also forgot most of how
ivi-layout works.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170127/eeddb859/attachment-0001.sig>