Hi Pekka, I will send a new patch after I fixed these issues. I used this commit message because it was used before by Tanibata-san.
If you want, I can send it also with another (shorter) commit message. Best regards Emre Ucan Software Group I (ADITG/SW1) > -----Original Message----- > From: Pekka Paalanen [mailto:[email protected]] > Sent: Donnerstag, 20. August 2015 11:44 > To: Ucan, Emre (ADITG/SW1) > Cc: [email protected]; Tanibata, Nobuhiko (ADITJ/SWG) > Subject: Re: [PATCH weston v3] ivi-shell: bugfix, an ivi_surface is not > removed from list of ivi_layer when the ivi_surface is removed from the > compositor. > > On Wed, 19 Aug 2015 11:25:02 +0000 > "Ucan, Emre (ADITG/SW1)" <[email protected]> wrote: > > > - Introduce the dirty parameter for triggering the render order change > > - IVI_NOTIFICATION_REMOVE/ADD flags are set only at commit_layer_list. > > > > Signed-off-by: Emre Ucan <[email protected]> > > Hi Ucan-san and Tanibata-san, > > there are a couple of commit message issues one should pay attention to in > the future. > > The email subject, a.k.a commit summary line, is too long. It should be > limited > to roughly around 60 characters or so. A good summary is unique, > descriptive, and short. It seems you are doing the opposite: > the summary is a long prose, while the message body has short bullet points. > > When you are sending v2, v3, etc. of a patch, it would be good to include a > list > of changes, see e.g. > http://cgit.freedesktop.org/wayland/weston/commit/?id=23ab7159d200883 > cc0d21db8dc4efdd58e2d60a7 > > Since we are now doing small patches to ivi-layout.c, I will be more strict on > wl_list manipulation. There is a lot to clean up here. > > (Btw. I am a little confused here on the architecture. If the purpose of > struct > link_layer is to allow an ivi-surface to be on multiple ivi-layers, how can a > direct list like struct ivi_layout_layer::order.surface_list work? Below it > looks > like struct ivi_layout_surface::order.link is what is stored in that list, > but you > cannot store the same link in multiple lists at the same time.) > > > --- > > ivi-shell/ivi-layout-private.h | 1 + > > ivi-shell/ivi-layout.c | 63 > > ++++++++++++++-------------------------- > > 2 files changed, 23 insertions(+), 41 deletions(-) > > > > diff --git a/ivi-shell/ivi-layout-private.h > > b/ivi-shell/ivi-layout-private.h index 9c04c30..074d598 100644 > > --- a/ivi-shell/ivi-layout-private.h > > +++ b/ivi-shell/ivi-layout-private.h > > @@ -81,6 +81,7 @@ struct ivi_layout_layer { > > } pending; > > > > struct { > > + int dirty; > > struct wl_list surface_list; > > struct wl_list link; > > } order; > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index > > d412069..bc8aead 100644 > > --- a/ivi-shell/ivi-layout.c > > +++ b/ivi-shell/ivi-layout.c > > @@ -809,53 +809,38 @@ commit_layer_list(struct ivi_layout *layout) > > > > ivilayer->prop = ivilayer->pending.prop; > > > > - if (!(ivilayer->event_mask & > > - (IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE)) > ) { > > + if (!ivilayer->order.dirty) { > > continue; > > } > > > > - 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); > > + 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; > > + if (!wl_list_empty(&ivisurf->order.link)) { > > + wl_list_remove(&ivisurf->order.link); > > While at it, let's fix these mis-patterns that give a false sense of safety. > > wl_list_empty() is not a valid way to check if a link can be > wl_list_remove()'d. > > Checking wl_list_empty() on a link offers no information: if it returns true, > wl_list_remove() is safe to do. If it returns false, you still do not know if > wl_list_remove() is safe; the link could be part of a list, or the link could > be > "uninitialized" (e.g. just wl_list_remove()'d). > > You need some other information to know whether a link is safe to remove. > It could be another variable, or it could be a design decision > (invariant) that it must be always safe to remove. > > As the wl_list_empty() check is completely bogus here, you can just remove > it and do the wl_list_remove() unconditionally. If it leads to a crash, the > code > was already broken before. I also do not think it can lead to a crash, because > the only case when the check avoids calling > wl_list_remove() is the only case according to wl_list_empty() that removing > actually is safe. > > > } > > > > - wl_list_init(&ivilayer->order.surface_list); > > + wl_list_init(&ivisurf->order.link); > > + ivisurf->event_mask |= > IVI_NOTIFICATION_REMOVE; > > } > > > > - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) { > > - 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(&ivilayer->order.surface_list); > > This wl_list_init() is confusing. Either the wl_list_for_each_safe loop above > removes all items from the list in which case the init is redundant, or if > there > are items still in the list then those items are left in a broken list. The > list also > cannot be "uninitialized" because we just iterated through it. > > If you want to ensure and document that the list really is empty at this > point, > do this: > assert(wl_list_empty(&ivilayer->order.surface_list)); > > > > > + wl_list_for_each(ivisurf, &ivilayer->pending.surface_list, > > + pending.link) { > > + if (!wl_list_empty(&ivisurf->order.link)){ > > + wl_list_remove(&ivisurf->order.link); > > Here's another bogus empty check. > > > wl_list_init(&ivisurf->order.link); > > Init before insert is redundant. wl_list_insert() does not read the link > before > overwriting it, so it is ok if the link is "uninitialized". > > > } > > > > - wl_list_init(&ivilayer->order.surface_list); > > - wl_list_for_each(ivisurf, &ivilayer- > >pending.surface_list, > > - pending.link) { > > - if (!wl_list_empty(&ivisurf->order.link)) { > > - wl_list_remove(&ivisurf->order.link); > > - wl_list_init(&ivisurf->order.link); > > - } > > - > > - wl_list_insert(&ivilayer->order.surface_list, > > - &ivisurf->order.link); > > - add_ordersurface_to_layer(ivisurf, ivilayer); > > - ivisurf->event_mask |= > IVI_NOTIFICATION_ADD; > > - } > > + wl_list_insert(&ivilayer->order.surface_list, > > + &ivisurf->order.link); > > + add_ordersurface_to_layer(ivisurf, ivilayer); > > + ivisurf->event_mask |= IVI_NOTIFICATION_ADD; > > } > > + > > + ivilayer->order.dirty = 0; > > } > > } > > > > @@ -997,8 +982,6 @@ clear_surface_pending_list(struct ivi_layout_layer > > *ivilayer) > > > > wl_list_init(&surface_link->pending.link); > > } > > - > > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE; > > } > > > > static void > > @@ -1015,8 +998,6 @@ clear_surface_order_list(struct ivi_layout_layer > > *ivilayer) > > > > wl_list_init(&surface_link->order.link); > > } > > - > > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE; > > } > > > > static void > > @@ -2102,7 +2083,7 @@ ivi_layout_layer_set_render_order(struct > ivi_layout_layer *ivilayer, > > } > > } > > > > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD; > > + ivilayer->order.dirty = 1; > > > > return IVI_SUCCEEDED; > > } > > @@ -2526,7 +2507,7 @@ ivi_layout_layer_add_surface(struct > ivi_layout_layer *ivilayer, > > } > > } > > > > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD; > > + ivilayer->order.dirty = 1; > > > > return IVI_SUCCEEDED; > > } > > @@ -2554,7 +2535,7 @@ ivi_layout_layer_remove_surface(struct > ivi_layout_layer *ivilayer, > > } > > } > > > > - remsurf->event_mask |= IVI_NOTIFICATION_REMOVE; > > + ivilayer->order.dirty = 1; > > } > > > > static int32_t > > Other than the mentioned above, the patch looks good to me. Using 'dirty' > instead of those flags sounds sensible, though I'm not too familiar with the > architecture here. > > > Thanks, > pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
