On Sun, Aug 6, 2017 at 11:11 PM, Jonas Ådahl <jad...@gmail.com> 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 <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.
>
> 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 <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
> > > > >
> > >
>

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
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to