<div dir="ltr">I'd find it a little bit inconvenient for the title, class, and app-id to be treated as buffered state. They make a great poor-man's way of identifying surfaces for embedded systems that implement simple layer management policies without wholesale adopting IVI shell (whose lack of wl_shell makes it difficult to integrate off-the-rack Wayland libraries and apps). Since the surface title doesn't have anything to do with correct graphical presentation, it'd be nicer if that data is just available immediately to the rest of the shell rather than being something that only trickles in whenever the app happens to post a buffer next.</div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 25, 2017 at 12:17 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Jul 21, 2017 at 05:21:38PM +0200, Quentin Glidic wrote: > On 7/20/17 4:33 PM, Matt Hoosier wrote: > > It's useful for the shell implementation to know when these change, > > for example to relay the information on to taskbars or similar. > > The idea is good, we need something like that. (But maybe not, see below). > > > > To avoid ABI changes or the need to make the weston_desktop_surface > > definition public, new functions are introduced for attaching > > listeners to these signals. > > We don’t care that much about ABI changes, but you’re right that I do care > about the struct staying private. > > > > Signed-off-by: Matt Hoosier <<a href="mailto:matt.hoosier@gmail.com">matt.hoosier@gmail.com</a>> > > --- > > libweston-desktop/libweston-desktop.h | 6 ++++++ > > libweston-desktop/surface.c | 21 +++++++++++++++++++++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/libweston-desktop/libweston-desktop.h b/libweston-desktop/libweston-desktop.h > > index 03b04c7..e38257e 100644 > > --- a/libweston-desktop/libweston-desktop.h > > +++ b/libweston-desktop/libweston-desktop.h > > @@ -164,6 +164,12 @@ weston_desktop_surface_set_size(struct weston_desktop_surface *surface, > > int32_t width, int32_t height); > > void > > weston_desktop_surface_close(struct weston_desktop_surface *surface); > > +void > > +weston_desktop_surface_add_title_listener(struct weston_desktop_surface *surface, > > + struct wl_listener *listener); > > +void > > +weston_desktop_surface_add_app_id_listener(struct weston_desktop_surface *surface, > > + struct wl_listener *listener); > > void * > > weston_desktop_surface_get_user_data(struct weston_desktop_surface *surface); > > diff --git a/libweston-desktop/surface.c b/libweston-desktop/surface.c > > index d3be936..97a455c 100644 > > --- a/libweston-desktop/surface.c > > +++ b/libweston-desktop/surface.c > > @@ -64,6 +64,8 @@ struct weston_desktop_surface { > > char *title; > > char *app_id; > > pid_t pid; > > + struct wl_signal title_signal; > > + struct wl_signal app_id_signal; > > }; > > struct { > > struct weston_desktop_surface *parent; > > @@ -287,6 +289,9 @@ weston_desktop_surface_create(struct weston_desktop *desktop, > > wl_list_init(&surface->view_list); > > wl_list_init(&surface->grab_link); > > + wl_signal_init(&surface->title_signal); > > + wl_signal_init(&surface->app_id_signal); > > + > > return surface; > > } > > @@ -511,6 +516,20 @@ weston_desktop_surface_close(struct weston_desktop_surface *surface) > > surface->implementation_data); > > } > > +WL_EXPORT void > > +weston_desktop_surface_add_title_listener(struct weston_desktop_surface *surface, > > + struct wl_listener *listener) > > +{ > > + wl_signal_add(&surface->title_signal, listener); > > +} > > + > > +WL_EXPORT void > > +weston_desktop_surface_add_app_id_listener(struct weston_desktop_surface *surface, > > + struct wl_listener *listener) > > +{ > > + wl_signal_add(&surface->app_id_signal, listener); > > +} > > + > > struct weston_desktop_surface * > > weston_desktop_surface_from_client_link(struct wl_list *link) > > { > > @@ -687,6 +706,7 @@ weston_desktop_surface_set_title(struct weston_desktop_surface *surface, > > free(surface->title); > > surface->title = tmp; > > + wl_signal_emit(&surface->title_signal, surface->title); > > I would rather pass the surface as the signal data. Just in case we have in > the future a value that doesn’t fit in a pointer. And calling a getter is > not that bad. > > Also, I prefer to have one signal only, it would reduce the API size (and > avoid it growing too much in the future, since it could work in this case). > We can move the free() to keep both pointer around until after the signal > fired, and a simple pointer comparison will work (if you stored the "const > char *" directly, which you should as with the signal, we guarantee it will > exist until destroy or signal). > > > But it raised a question : Jonas, are set_title() and set_app_id() supposed > to be active without a commit()? </div></div>No. As per specification they are not tied to wl_surface.commit(). I suppose it is something we can change though, while we can, if it makes any sense. Opinions? Jonas </blockquote></div> </div>