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. > > 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 <[email protected]> > > --- > > 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
