[PATCH weston 2/7] xwayland: add set_toplevel_with_position to internal API

[Pekka Paalanen]

On Tue, 29 Nov 2016 20:23:29 +0100
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 29/11/2016 16:11, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Add a new entry to the internal interface between the xwayland plugin
> > and libweston-desktop (or any other desktop protocol implementation).
> > The new entry is identical to set_toplevel except it carries an absolute
> > position for the toplevel window.
> >
> > Following patches will implement this new entry in
> > libweston-desktop and start using it in XWM.
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  xwayland/xwayland-internal-interface.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/xwayland/xwayland-internal-interface.h b/xwayland/xwayland-internal-interface.h
> > index e771730..1096444 100644
> > --- a/xwayland/xwayland-internal-interface.h
> > +++ b/xwayland/xwayland-internal-interface.h
> > @@ -38,6 +38,8 @@ struct weston_desktop_xwayland_interface {
> >  						      struct weston_surface *surface,
> >  						      const struct weston_xwayland_client_interface *client);
> >  	void (*set_toplevel)(struct weston_desktop_xwayland_surface *shsurf);
> > +	void (*set_toplevel_with_position)(struct weston_desktop_xwayland_surface *shsurf,
> > +					   int32_t x, int32_t y);  
> 
> Why not "set_position"? It seems cleaner to me, and potentially usable 
> for other types. Since this is an internal detail, we should be safe 
> from bad code to call it too much.

Hi,

because then I would have to explain when set_position() should be
called and when it cannot be called, plus add all the code to ensure it
is not misused. It does not matter if it's "internal code" when it is
at the interface between two different components.

Setting the position is not something you can do at arbitrary times or
with arbitrary window types. It must always be done in connection with
set_toplevel and immediately after. Any other use for it I have not
considered and would not know if or how it might work. I want to avoid
that extra design burden.

Combining both into a single function call makes the API impossible to
misuse in that respect.

If we actually do end up wanting a set_position for other types, we can
revisit this design then. However, even then, if set_position does not
apply to most window types, cannot be used at arbitrary times, and
there are not more things than position to set, I would still consider
just adding another set_<type>_with_position().

Would you agree?

OTOH, in patch 3, struct weston_desktop_api does not have a "set
toplevel" so I cannot do the same combination there. But
weston_desktop_api is only applicable to toplevels, isn't it?


Thanks,
pq

> Patch 4 would need an update then.
> 
> Cheers,
> 
> 
> >  	void (*set_parent)(struct weston_desktop_xwayland_surface *shsurf,
> >  			   struct weston_surface *parent);
> >  	void (*set_transient)(struct weston_desktop_xwayland_surface *shsurf,
> >  
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161130/ec123b72/attachment.sig>