Hi Pekka, > -----Original Message----- > From: wayland-devel [mailto:wayland-devel- > [email protected]] On Behalf Of Pekka Paalanen > Sent: Donnerstag, 20. August 2015 12:43 > To: Ucan, Emre (ADITG/SW1) > Cc: Tanibata, Nobuhiko (ADITJ/SWG); [email protected] > Subject: Re: [PATCH weston] ivi-shell: bugfix, list of layers on a screen are > cumulated when set render order is called several time in one > commitchanges. > > On Wed, 19 Aug 2015 11:25:03 +0000 > "Ucan, Emre (ADITG/SW1)" <[email protected]> wrote: > > > - Always clear pending list at set_render_order API. > > - Introduce the dirty parameter for triggering the render order change. > > - IVI_NOTIFICATION_REMOVE/ADD flags are set only at > commit_screen_list. > > Hi, > > the same comments about the commit message and summary as earlier. > > Essentially all my comments here are just references to the comments I > made in http://lists.freedesktop.org/archives/wayland-devel/2015- > August/023987.html .
I will do the changes according to your comments. I can also send the new patch with a shorter commit message if you prefer. > > > > > Signed-off-by: Emre Ucan <[email protected]> > > --- > > ivi-shell/ivi-layout.c | 45 +++++++++++---------------------------------- > > 1 file changed, 11 insertions(+), 34 deletions(-) > > > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index > > bc8aead..898a901 100644 > > --- a/ivi-shell/ivi-layout.c > > +++ b/ivi-shell/ivi-layout.c > > @@ -93,14 +93,13 @@ struct ivi_layout_screen { > > struct ivi_layout *layout; > > struct weston_output *output; > > > > - uint32_t event_mask; > > - > > struct { > > struct wl_list layer_list; > > struct wl_list link; > > } pending; > > > > struct { > > + int dirty; > > struct wl_list layer_list; > > struct wl_list link; > > } order; > > @@ -431,7 +430,6 @@ create_screen(struct weston_compositor *ec) > > count++; > > > > iviscrn->output = output; > > - iviscrn->event_mask = 0; > > > > wl_list_init(&iviscrn->pending.layer_list); > > wl_list_init(&iviscrn->pending.link); > > @@ -853,7 +851,7 @@ commit_screen_list(struct ivi_layout *layout) > > struct ivi_layout_surface *ivisurf = NULL; > > > > wl_list_for_each(iviscrn, &layout->screen_list, link) { > > - if (iviscrn->event_mask & IVI_NOTIFICATION_REMOVE) { > > + if (iviscrn->order.dirty) { > > wl_list_for_each_safe(ivilayer, next, > > &iviscrn->order.layer_list, > order.link) { > > remove_orderlayer_from_screen(ivilayer); > > @@ -865,21 +863,9 @@ commit_screen_list(struct ivi_layout *layout) > > wl_list_init(&ivilayer->order.link); > > ivilayer->event_mask |= > IVI_NOTIFICATION_REMOVE; > > } > > - } > > - > > - if (iviscrn->event_mask & IVI_NOTIFICATION_ADD) { > > - wl_list_for_each_safe(ivilayer, next, > > - &iviscrn->order.layer_list, > order.link) { > > - remove_orderlayer_from_screen(ivilayer); > > - > > - if (!wl_list_empty(&ivilayer->order.link)) { > > - wl_list_remove(&ivilayer- > >order.link); > > - } > > - > > - wl_list_init(&ivilayer->order.link); > > - } > > > > wl_list_init(&iviscrn->order.layer_list); > > assert(wl_list_empty(&iviscrn->order.layer_list)); > > > + > > wl_list_for_each(ivilayer, &iviscrn- > >pending.layer_list, > > pending.link) { > > wl_list_insert(&iviscrn->order.layer_list, > > @@ -887,9 +873,9 @@ commit_screen_list(struct ivi_layout *layout) > > add_orderlayer_to_screen(ivilayer, iviscrn); > > ivilayer->event_mask |= > IVI_NOTIFICATION_ADD; > > } > > - } > > > > - iviscrn->event_mask = 0; > > + iviscrn->order.dirty = 0; > > + } > > > > /* Clear view list of layout ivi_layer */ > > wl_list_init(&layout->layout_layer.view_list.link); > > (Btw. as a side comment here, this init is dangerous. If this link was a part > of > any list, that list is now broken. If the code happens to do a > wl_list_remove() on a link that was adjacent to this link before the init, > then > this link will break too. It's not obvious (at least to me) that the link > cannot be > in any list or that all members of the list are reset the same way, including > the > head.) > Thank you for remark. For now I think it is ok, because the layout_layer is only modified within commit_screen_list(). Therefore, its view_list is not a part of another list etc. > > @@ -2330,7 +2316,7 @@ ivi_layout_screen_add_layer(struct > ivi_layout_screen *iviscrn, > > } > > } > > > > - iviscrn->event_mask |= IVI_NOTIFICATION_ADD; > > + iviscrn->order.dirty = 1; > > > > return IVI_SUCCEEDED; > > } > > @@ -2353,24 +2339,15 @@ ivi_layout_screen_set_render_order(struct > > ivi_layout_screen *iviscrn, > > > > wl_list_for_each_safe(ivilayer, next, > > &iviscrn->pending.layer_list, pending.link) { > > + if (!wl_list_empty(&ivilayer->pending.link)) { > > This is a bogus check, as explained in my other review. > > > + wl_list_remove(&ivilayer->pending.link); > > + } > > + > > wl_list_init(&ivilayer->pending.link); > > } > > > > wl_list_init(&iviscrn->pending.layer_list); > > assert(wl_list_empty(&iviscrn->pending.layer_list)); > > > > > - if (pLayer == NULL) { > > - wl_list_for_each_safe(ivilayer, next, &iviscrn- > >pending.layer_list, pending.link) { > > - if (!wl_list_empty(&ivilayer->pending.link)) { > > - wl_list_remove(&ivilayer->pending.link); > > - } > > - > > - wl_list_init(&ivilayer->pending.link); > > - } > > - > > - iviscrn->event_mask |= IVI_NOTIFICATION_REMOVE; > > - return IVI_SUCCEEDED; > > - } > > - > > for (i = 0; i < number; i++) { > > id_layer = &pLayer[i]->id_layer; > > wl_list_for_each(ivilayer, &layout->layer_list, link) { @@ - > 2388,7 > > +2365,7 @@ ivi_layout_screen_set_render_order(struct ivi_layout_screen > *iviscrn, > > } > > } > > > > - iviscrn->event_mask |= IVI_NOTIFICATION_ADD; > > + iviscrn->order.dirty = 1; > > > > return IVI_SUCCEEDED; > > } > > Essentially a good patch, needs a little work on few details and the commit > message. > > > Thanks, > pq Best regards Emre Ucan Software Group I (ADITG/SW1) _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
