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

[Ucan, Emre (ADITG/SW1)]

Hello Pekka,

Thank your for review.

When a view is just made invisible, the view below should be damaged.
Normally, this should be done automatically in weston_view_update_transform.
But invisinble views are not in compositor's view list. Therefore, they are not updated.
This patch explicitly check, If the layer or surface of the ivi_view is made invisible in
this commit_changes call. Then, it calls weston_view_damage_below.

Some other comments below:


> -----Original Message-----
> From: wayland-devel [mailto:wayland-devel-
> bounces at lists.freedesktop.org] On Behalf Of Pekka Paalanen
> Sent: Freitag, 27. Januar 2017 15:34
> To: Ucan, Emre (ADITG/SW1)
> Cc: wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH weston] ivi-shell: Damage view below after unmapping
> 
> 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?

I did this change because of style considerations. It is better to use one for loop.
Instead of three nested loops. Especially, if tabs are 8 spaces (:
But you are right. It should send a different patch to fix this style issue.

> 
> 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?

Yes, ivi_view_create function always creates an ivi_view with a non NULL ivilayer and ivisurf.

> 
> >
> > -			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 is skipped in current implementation too, if the surface or layer is invisible.
See:
                                /*
                                   * If ivilayer is invisible, weston_view of ivisurf doesn't
                                   * need to be modified.
                                   */
                                  if (ivi_view->ivisurf->prop.visibility == false)
                                          continue;

> 
> >  		}
> > +
> > +		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

I will send a more simpler patch, which only fixes this issue.

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937