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

[Pekka Paalanen]

On Fri, 27 Jan 2017 15:11:41 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:

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

Hi,

it's not just a style issue. You are iterating a completely different
list. Therefore the commit message should explain what is the
difference between the lists and consider what other things it will
change implicitly...

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

...like this one.

As I don't have the ivi-layout workings in my memory, it would help
review if these details were written down, so it remains to just check
them.

> > > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > > ---
> > >  ivi-shell/ivi-layout.c |   41 ++++++++++++++++++++++++-----------------
> > >  1 file changed, 24 insertions(+), 17 deletions(-)

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

Ah, ok, it's an invariant.


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

Oh, yes indeed.

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

Very good.

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/9779ac89/attachment.sig>