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

[Quentin Glidic]

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()?

If so, maybe should we test equality in libweston-desktop, to avoid 
sending useless signals? (Just in case a client would be lazy and send 
the title on every frame.)
And I would like to know if one signal sounds good to you?

If *not*, then the committed callback is already the place to check for 
changes, and we need to adjust the code to match that. (And to keep old 
pointers around for safe comparison.)


Cheers,

>   }
>   
>   void
> @@ -701,6 +721,7 @@ weston_desktop_surface_set_app_id(struct weston_desktop_surface *surface,
>   
>   	free(surface->app_id);
>   	surface->app_id = tmp;
> +	wl_signal_emit(&surface->app_id_signal, surface->app_id);
>   }
>   
>   void
> 


-- 

Quentin “Sardem FF7” Glidic