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

[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.

> 
> >> +
> >> +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.

> >> +    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.

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


Thanks,
pq