[PATCH weston-ivi-shell v5 2/9] The weston-layout library supports

[Nobuhiko Tanibata]

Hi,

I apply review comments as v5 except following comments.

>> +
>> +struct link_layerPropertyNotification {
>> +    layerPropertyNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_surfacePropertyNotification {
>> +    surfacePropertyNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_layerCreateNotification {
>> +    layerCreateNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_layerRemoveNotification {
>> +    layerRemoveNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_surfaceCreateNotification {
>> +    surfaceCreateNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_surfaceRemoveNotification {
>> +    surfaceRemoveNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
>> +
>> +struct link_surfaceConfigureNotification {
>> +    surfaceConfigureNotificationFunc callback;
>> +    void *userdata;
>> +    struct wl_list link;
>> +};
> 
> Any reason you defined all these different structs, and didn't use the
> wl_signal/wl_listener pattern?
> 

I wants callback with some information. I think wl_signal/wl_listener 
can not give such a information. So I keep this. However if there is an 
another solution with wl_signal/wl_listener, I promise I would fix this 
code.

>> +
>> +struct weston_layout;
>> +
>> +struct weston_layout_surface {
>> +    struct wl_list link;
> 
> For link members, I would personally prefer some indication of which
> (kind of) list the link will belong to, if possible and especially if
> there are multiple links in a struct.
> 
>> +    struct wl_list list_notification;
>> +    struct wl_list list_layer;
> 
> The Weston convention is to say "notification_list", "layer_list". I
> would also add a comment saying what link the list will hold, e.g.
> ... foo_list; /* struct weston_layout_layer::link */
> That explicitly says what the struct embedding the 'link' member is, as
> one list can only ever contain one type of objects, and explains how to
> access that object from a link pointer.
> 
> Hmm... so one surface can be in multiple layers??

Yes. There is a use case to display a surface to several layer. E.g. 
lane guidance can be shown in a small panel in cluster.

> 
>> +    uint32_t update_count;
>> +    uint32_t id_surface;
>> +
>> +    struct weston_layout *layout;
>> +    struct weston_surface *surface;
>> +    struct weston_view *view;
> 
> Why have the view pointer explicitly here? Can't you access that via
> weston_surface?
> 
>> +
>> +    uint32_t buffer_width;
>> +    uint32_t buffer_height;
> 
> Would these perhaps happen to be the same as
> weston_surface::width_from_buffer and
> weston_surface::height_from_buffer?
> 

I promise I will fix it soon.

> 
> Do you handle at all the case of hotplugging outputs?
> Does the IVI software architecture simply not support any kind of
> hotplug?
> 

No. there is no use case like hot plug.

> 
> Error, or silently ignored?
> Do you plan to implement this function?
> 
> There are a lot more similar functions which have the same "not
> supported" comment.
> 
> ...

Yes. I will plan to support soon.


> 
> All these unsigned members seem like they are calling for mismatching
> signed and unsigned variables in computations, which can lead to
> surprising bugs. Is there a reason why these coordinate related members
> (position and size) are unsigned?
> 
> Weston code base and even Wayland protocol categorically uses signed
> integers for everything that is computed with, like size which can
> never be legally negative. Only flags, bitfields, object ids and such
> are unsigned, because they will never be used in computations that may
> include signed integers.
> 

Yes. you are right. I have to modify many things. I promise I will fix 
this next steps.

BR,
Nobuhiko