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. > > >> + > >> +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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel