On Sun, Aug 6, 2017 at 11:11 PM, Jonas Ådahl <[email protected]> wrote:
> On Tue, Jul 25, 2017 at 08:07:01AM -0600, Matt Hoosier wrote: > > On Tue, Jul 25, 2017 at 7:26 AM, Jonas Ådahl <[email protected]> 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 > > > > > > > > > > > > > > Jonas > > > > > > > > > > > On Tue, Jul 25, 2017 at 12:17 AM, Jonas Ådahl <[email protected]> > 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 <[email protected]> > > > > > > > --- > > > > > > > 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 > > > > > > > > > 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?
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
