On Fri, 27 Jan 2017 15:11:41 +0000 "Ucan, Emre (ADITG/SW1)" <[email protected]> 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- > > [email protected]] On Behalf Of Pekka Paalanen > > Sent: Freitag, 27. Januar 2017 15:34 > > To: Ucan, Emre (ADITG/SW1) > > Cc: [email protected] > > Subject: Re: [PATCH weston] ivi-shell: Damage view below after unmapping > > > > On Tue, 17 Jan 2017 12:30:56 +0000 > > "Ucan, Emre (ADITG/SW1)" <[email protected]> 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 <[email protected]> > > > --- > > > 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
pgpGdLCxAdO8G.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
