[PATCH] libweston-desktop: add signals for title, app_id
Matt Hoosier
matt.hoosier at gmail.com
Thu Aug 3 16:24:49 UTC 2017
Hi,
Any further consensus on this?
On Tue, Jul 25, 2017 at 8:07 AM, Matt Hoosier <matt.hoosier at gmail.com>
wrote:
> On Tue, Jul 25, 2017 at 7:26 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
>
>> On Tue, Jul 25, 2017 at 06:42:34AM -0600, Matt Hoosier wrote:
>> > 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.
>>
>> There is no need to attach a new buffer to commit a new state. One could
>> for example commit a state only changing the title and nothing else, and
>> it'd be completely correct. The benefit of synchronizing it is for
>> example that a shell can synchronously change some overlay with the
>> content.
>>
>
> Yes, agreed. But that's not the way that the Wayland backends of big
> programming frameworks like Qt, Gtk+, etc work. In practice, you need to
> trigger rendering if you want a commit to happen.
>
>
>>
>> Regarding the app_id, the app_id should normally only ever be set once,
>> as part of the initial state that doesn't have a buffer attached yet.
>>
>
> That's a fair point. The title can still change at will though, can't it?
> Isn't the regular old window title what gets used to show things like the
> working directly in an xterm or the name of the currently-edited document
> in a word processor?
>
>
>>
>>
>> Jonas
>>
>> >
>> > On Tue, Jul 25, 2017 at 12:17 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
>> >
>> > > 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
>> > >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170803/3029f73b/attachment.html>
More information about the wayland-devel
mailing list