> -----Original Message----- > From: wayland-devel > [mailto:[email protected]] On Behalf Of Pekka > Paalanen > Sent: Thursday, August 13, 2015 10:21 PM > To: Nobuhiko Tanibata > Cc: [email protected]; [email protected] > Subject: Re: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not > removed from list of ivi_layer when the ivi_surface is removed from the > compositor. > > On Fri, 7 Aug 2015 09:47:02 +0900 > Nobuhiko Tanibata <[email protected]> wrote: > > > The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface > > from　a list of ivi_layer. In previous code, there is no trigger to > > refresh order of list, removing the ivi_surface, in commit_layer_list. > > > > To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to > > trigger refresh list of surface in commit_layer_list. > > > > In commit_layer_list, this patch also removes duplicated code in two > > conditions for IVI_NOTIFICATION_ADD/REMOVE. > > > > Signed-off-by: Nobuhiko Tanibata <[email protected]> > > --- > > v2 changes: > > - fix 8 spaces to tab. > > - clean up duplicate code in commit_layer_list. > > - improve commit message. > > > > ivi-shell/ivi-layout.c | 28 +++++++++------------------- > > 1 file changed, 9 insertions(+), 19 deletions(-) > > > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index > > 2974bb7..1b45003 100644 > > --- a/ivi-shell/ivi-layout.c > > +++ b/ivi-shell/ivi-layout.c > > @@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout) > > if (!(ivilayer->event_mask & > > (IVI_NOTIFICATION_ADD | > IVI_NOTIFICATION_REMOVE)) ) { > > continue; > > Hi Tanibata-san, > > using 'continue', there is no need to have an else-branch. This would save > one level of indent in the remaining code. > > > - } > > - > > - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) { > > - wl_list_for_each_safe(ivisurf, next, > > - &ivilayer->order.surface_list, > order.link) { > > - > remove_ordersurface_from_layer(ivisurf); > > - > > - if > (!wl_list_empty(&ivisurf->order.link)) { > > - > wl_list_remove(&ivisurf->order.link); > > - } > > - > > - wl_list_init(&ivisurf->order.link); > > - ivisurf->event_mask |= > IVI_NOTIFICATION_REMOVE; > > You are removing this setting of the flag IVI_NOTIFICATION_REMOVE, but in > the code that remains I do not see that happening anymore in > commit_layer_list(). > > However, I see it being set by ivi_layout_layer_remove_surface(). At which > time should the flag be set? Should the ADD flag not work the same way? > > Would it be essential to flag ivi-surfaces that get removed from > ivilayer->order.surface_list with IVI_NOTIFICATION_REMOVE, and surfaces > that are added to flag with IVI_NOTIFICATION_ADD, and all remaining surfaces > even if they are reordered not be flagged at all? > > Or is it intended that all surfaces that are originally in the > ivilayer->order.surface_list are flagged as REMOVE, and all surfaces in > the pending list are flagged as ADD? That would imply that surfaces that > were and still remain in the order list are flagged as both REMOVE and ADD. > > > - } > > - > > - wl_list_init(&ivilayer->order.surface_list); > > - } > > - > > - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) { > > + } else { > > wl_list_for_each_safe(ivisurf, next, > > > &ivilayer->order.surface_list, order.link) { > > > remove_ordersurface_from_layer(ivisurf); > > @@ -843,6 +825,13 @@ commit_layer_list(struct ivi_layout *layout) > > } > > > > wl_list_init(&ivilayer->order.surface_list); > > + > > + /** > > + * Following ivilayer->pending.surface_list must > be maintained by > > + * a function who will set these masks. Order of > surfaces in a > > + * layer is restructured here. if there is no > surface in > > + * surface_list, the following code is skipped. > > + */ > > wl_list_for_each(ivisurf, > &ivilayer->pending.surface_list, > > pending.link) { > > > if(!wl_list_empty(&ivisurf->order.link)){ > > @@ -2565,6 +2554,7 @@ ivi_layout_layer_remove_surface(struct > ivi_layout_layer *ivilayer, > > } > > > > remsurf->event_mask |= IVI_NOTIFICATION_REMOVE; > > + ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE; > > } > > > > static int32_t > > All the flagging issues aside, I see what this patch does. > > Previously removing a ivi-surface didn't work at all. With this patch, > whether ivi-surfaces are added or removed from the ivi-layer, the code will > always completely reconstruct the ivilayer->order.surface_list from the > pending list. In that sense: > > Reviewed-by: Pekka Paalanen <[email protected]> > > Do you want me to push this patch as is? [ntanibata] Hi Pekka-san, Thank you for review. No, I don't. I shall reconsider name of masks: IVI_NOTIFICATION_REMOVE/ADD.
BR, Nobuhiko Tanibata > > > Thanks, > pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
