[PATCH] libweston-desktop: add signals for title, app_id

[Jonas Ådahl]

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 <matt.hoosier at gmail.com>
> > ---
> >   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()?

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