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

[Matt Hoosier]

On Fri, Jul 21, 2017 at 10:21 AM, Quentin Glidic <
sardemff7+wayland at sardemff7.net> 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).
>

I'm fine with using just one signal to announce all this. What sort of name
seems appropriate? "name_signal" maybe?


>
>
> 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
>

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170721/c4792626/attachment.html>