Hi, Any further consensus on this?
On Tue, Jul 25, 2017 at 8:07 AM, Matt Hoosier <matt.hoos...@gmail.com> wrote: > On Tue, Jul 25, 2017 at 7:26 AM, Jonas Ådahl <jad...@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 <jad...@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.hoos...@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 >> > > >> > >
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel