[PATCH weston 3/6] libweston-desktop/xwayland: window type XWAYLAND cannot have a parent

[Pekka Paalanen]

On Thu, 24 Nov 2016 15:04:08 +0100
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 24/11/2016 13:59, Pekka Paalanen wrote:
> > On Thu, 24 Nov 2016 12:29:26 +0000
> > Daniel Stone <daniel at fooishbar.org> wrote:
> >  
> >> Hi,
> >>
> >> On 24 November 2016 at 11:40, Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> >>> Add an assert to ensure that a window of type XWAYLAND is never
> >>> attempted with a parent.
> >>>
> >>> This essentially adding documentation.
> >>>
> >>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >>> ---
> >>>  libweston-desktop/xwayland.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/libweston-desktop/xwayland.c b/libweston-desktop/xwayland.c
> >>> index bd68bc6..0c4ff2b 100644
> >>> --- a/libweston-desktop/xwayland.c
> >>> +++ b/libweston-desktop/xwayland.c
> >>> @@ -74,6 +74,7 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
> >>>         bool to_add = (parent == NULL && state != XWAYLAND);
> >>>
> >>>         assert(state != NONE);
> >>> +       assert(!parent || state != XWAYLAND);  
> >>
> >> From looking at the callers, it seems like it should be (!parent ||
> >> state == TRANSIENT). I'm a huge fan of adding documentation via
> >> asserts though!  
> >
> > Hi,
> >
> > hm, Quentin said type XWAYLAND windows (mostly just override-redirect)
> > must not have a parent, and that's what I wanted to document since
> > XWAYLAND is the one with all the special handling.  
> 
> 
> That is exactly why I don’t like asserts as docs for such cases.
> 
> This is all internal API, and it is supposed to be used that way:
> - TOPLEVEL, MAXIMIZED and FULLSCREEN are for toplevel windows, that the 
> shell will know about
> - XWAYLAND is for special parentless windows (mostly O-R, but currently 
> we have some false positive matches)
> - TRANSIENT is for grabless windows that the shell will never know 
> about, and all of them will have a parent *and a position relative to 
> this parent*

That would have made a pretty nice doxygen doc. ;-)

I didn't realize TRANSIENT windows are never "added". I would have
assumed to have spotted it in the code... or did I misunderstand
"added" again?

> If a window has a parent and a relative positione, it must be linked to 
> this parent. This way, any created view of the parent will get children 
> views created too. Theoretically, an XWAYLAND window could have O-R 
> children, if they do not have an absolute position, but they would have 
> to be TRANSIENT.
> 
> Both your asserts are correct, but they do not “document” the same 
> thing. :-)

In that case, let's go with the more strict assert. Asserts can always
be relaxed later, making them more strict might not be easy.


Thanks,
pq

> Maybe it was a mistake to reuse as much code as possible here, since 
> XWAYLAND is really special.
> 
> Cheers,
> 

-------------- 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/20161124/002e08d5/attachment-0001.sig>