Hi Pekka, Yes, it makes sense to squash patches. I will also add the missing documentation about the listener APIs. I will send modified patches soon. Thank you for your review.
> -----Original Message----- > From: Pekka Paalanen [mailto:[email protected]] > Sent: Freitag, 1. April 2016 13:44 > To: Ucan, Emre (ADITG/SW1) > Cc: [email protected]; Natsume, Wataru (ADITJ/SWG) > Subject: Re: [PATCH weston 00/16] IVI Layout Notification APIs Rework > > On Thu, 31 Mar 2016 11:08:46 +0000 > "Ucan, Emre (ADITG/SW1)" <[email protected]> wrote: > > > I changed the interface of the notification APIs to get a wl_listener > > instead of a ivi-shell specific notification callback function. > > > > This change has several advantages: > > 1. Code cleanup > > 2. No dynamic memory allocation. Listeners are allocated > > by controller plugins > > 3. Remove API is not needed. Controller plugins can easily > > remove the listener link. > > > > The first patch is a preparation for the notification API changes. > > It moves the event_mask to ivi_layout_surface/layer_properties struct, > > so that it is accessible by get_properties_of_layer/surface APIs. > > Then, it is not needed to send event_mask with an ivi-shell callback > function. > > > > The patches 2-15 modifies and renames IVI Layout notification APIs, > > and removes their remove APIs. > > > > The 16th patch removes content_observer callback function which is a > > leftover of the patch: 193e301c740b582f20307e3e08f8373d26ea968f > > > > Emre Ucan (16): > > ivi-shell: move event_mask to properties struct > > ivi-shell: rework surface_add_notification API > > ivi-shell: remove surface_remove_notification APIs > > ivi-shell: rework layer_add_notification API > > ivi-shell: remove layer_remove_notification APIs > > ivi-shell: rework create_surface notification > > ivi-shell: remove remove_notification_create_surface > > ivi-shell: rework create_layer_notification > > ivi-shell: remove remove_notification_create_layer > > ivi-shell: rework remove_layer notification > > ivi-shell: remove remove_notification_remove_layer > > ivi-shell: rework remove_surface notification > > ivi-shell: remove remove_notification_remove_surface > > ivi-shell: rework configure_surface notification > > ivi-shell: remove remove_notification_configure_surface > > ivi-shell: remove content_observer leftover > > > > ivi-shell/hmi-controller.c | 42 +-- > > ivi-shell/ivi-layout-export.h | 113 +------- > > ivi-shell/ivi-layout-private.h | 2 - > > ivi-shell/ivi-layout.c | 545 > > +++++--------------------------------- > > tests/ivi_layout-internal-test.c | 76 +++--- > > tests/ivi_layout-test-plugin.c | 93 ++++--- > > 6 files changed, 209 insertions(+), 662 deletions(-) > > > > Hi, > > I had separate comments on patch 2, which probably needs some bits of > patch 3. > > Somehow I would assume that patch 4 would have a similar issue with > ivi_layout_layer_remove_notification() call being removed only in patch 5, > but it didn't trigger for me. > > Considering the add and remove notifier/listener are two sides of the same > thing, maybe it would be logical to do both in the same patch? > That is, squash 2 and 3, 4 and 5, etc.? > > For all the add_listener public API functions you should document what the > void *data argument to the wl_listener::notify callback is. > > In the great assignment block of static struct ivi_layout_interface > ivi_layout_interface it is ok to keep a comma also after the last item. > It helps keeping diffs one change smaller. > > It is not needed to add a cast to/from void*. > > Patches 1 and 16 are R-b me and pushed: > 87953b7..0bd29b6 master -> master > > Patches 2 - 15 I hope to see revised. > > > Thanks, > pq 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
