<div dir="ltr">Hi,<div> </div><div>Any further consensus on this?</div></div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 25, 2017 at 8:07 AM, Matt Hoosier <<a href="mailto:matt.hoosier@gmail.com" target="_blank">matt.hoosier@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 25, 2017 at 7:26 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">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. </blockquote><div> </div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote><div> </div><div>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?</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Jonas <div class="m_-5988336787100589592m_2858633519935362535HOEnZb"><div class="m_-5988336787100589592m_2858633519935362535h5"> > > On Tue, Jul 25, 2017 at 12:17 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>> 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 <<a href="mailto:matt.hoosier@gmail.com" target="_blank">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()? > > > > 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 > > </div></div></blockquote></div></div></div> </div></div> </blockquote></div> </div>