Hi Tanibata-san, Tel. +49 5121 49 6937 > -----Original Message----- > From: wayland-devel [mailto:wayland-devel- > boun...@lists.freedesktop.org] On Behalf Of Tanibata, Nobuhiko > (ADITJ/SWG) > Sent: Montag, 17. August 2015 08:01 > To: Pekka Paalanen; Nobuhiko Tanibata > Cc: Ishikawa, Tetsuri (ADITJ/SWG); securitych...@denso.co.jp; wayland- > de...@lists.freedesktop.org > 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. > > > > > -----Original Message----- > > From: wayland-devel > > [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of > > Pekka Paalanen > > Sent: Thursday, August 13, 2015 10:21 PM > > To: Nobuhiko Tanibata > > Cc: securitych...@denso.co.jp; wayland-devel@lists.freedesktop.org > > 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 <nobuhiko_tanib...@xddp.denso.co.jp> 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 > > > <nobuhiko_tanib...@xddp.denso.co.jp> > > > --- > > > 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 > > ivilayer->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 > > ivilayer->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 <pekka.paala...@collabora.co.uk> > > > > 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. >
I think it is bad design to use IVI_NOTIFICATION_REMOVE/ADD flags to trigger rendering order change at commit_layer_list. These notification flags are designed to notify hmi/ivi-controller about a changing property of a surface/layer. In my opinion, we can implement like this: - commit_layer_list() should clear the pending list after the pending list is copied to order list. - ivi_layout_layer_set_render_order should clear the pending list before setting a new one. - pending list should be controlled at commit_layer_list(). If it is empty, do nothing. If it is not empty, compare the old order list and pending list, set IVI_NOTIFICATION_ADD for added surfaces and REMOVE for removed surfaces. - The IVI_NOTIFICATION_REMOVE/ADD flags should only be set by commit_layer_list() function. If you want, I can prepare patches for this behavior. > BR, > Nobuhiko Tanibata > > > > > > Thanks, > > pq > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel Best regards Emre Ucan Software Group I (ADITG/SW1) _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel