<div dir="ltr">On Sun, Aug 6, 2017 at 11:11 PM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jul 25, 2017 at 08:07:01AM -0600, Matt Hoosier wrote: > On Tue, Jul 25, 2017 at 7:26 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> 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. Toolkits could make this work (i.e. just call wl_surface_commit() after setting the title), I'm sure of, but indeed it could complicate things, as they need to make sure not to set any pending state too early. For example gtk did this poorly before, but has since been improved. Not sure this slight complication is a reason to not apply the title on commit though. > > > > > > 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? Yes it can. > 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? Right. The potential purpose of making set_title etc be applied on commit would be to update for example the window title text in the gnome shell overview (or any equivalent presentation) at the exact same time as the content of the document is updated. Jonas <div class="HOEnZb"><div class="h5"> > > > > > > > > Jonas > > > > > > > > On Tue, Jul 25, 2017 at 12:17 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com">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">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 class="gmail_extra">Okay. Well, I'm not sure how you'd like this to proceed. Are you planning to update the spec to require that set_title and set_app_id are buffered? Or is that too much effort and you'd rather than libweston-desktop just gets a patch like what I proposed to allow the state-change to become visible when it arrives?</div></div>