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

[Nobuhiko Tanibata]

2014-07-07 16:10 に Pekka Paalanen さんは書きました:
> On Tue, 20 May 2014 13:52:55 +0900
> Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> 
>> 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.
> 
> Are you saying, that you could not see how to access a userdata
> pointer if you used wl_signal/wl_listener?
> 
> Here is an example how it works:
> http://cgit.freedesktop.org/wayland/weston/tree/src/compositor-drm.c#n2572
> 
> The recorder_frame_notify digs the drm_output pointer from the
> wl_listener, since the wl_listener is embedded in struct drm_output.
> 
> This is documented at wl_listener:
> http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-server.h#n132
> (Hrm, looks like there is a slight confusion in the example.)
> 
> So the trick is to use wl_container_of() on the listener pointer to
> get to your own data.
> 

OK, now I see. Thank you for your point! I try to do it.

>> 
>> >> +
>> >> +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.
> 
> Hmm, and you can't use weston_view instead?
> 
> One weston_surface can have several weston_views, and views are
> what you put in layers in weston core. Maybe I just don't remember
> what all is involved.
> 

IVI layer can group surfaces. It also has attribute its own viewport 
like clip region, opacity, orientation and son. Later one can not be 
realized by weston_view.
The code finally use view of each surface by referring "order and 
attribute of surface in a layer" and "order and attribute of layer".

Thank you,
ntanibata


>> >> +    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.
> 
> Okay.
> 
>> > 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.
> 
> Very good.

This was applied at v6 in terms of unsigned member.

BR,
ntanibata

> 
> Sorry, I still haven't even looked at your v6.
> 
> 
> Thanks,
> pq