2014-07-07 16:10 に Pekka Paalanen さんは書きました:
On Tue, 20 May 2014 13:52:55 +0900
Nobuhiko Tanibata <nobuhiko_tanib...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
