[PATCH weston 6/6] libweston-desktop/xwayland: add is_mapped handling for XWAYLAND

[Pekka Paalanen]

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

> On 24/11/2016 12:40, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > The xwayland window type XWAYLAND is not handled by the shell at all,
> > instead libweston-desktop maps such surfaces itself. However, it forgot
> > to set weston_surface::is_mapped and weston_view::is_mapped.
> >
> > weston_surface::is_mapped affects the behaviour of weston_view_unmap()
> > and weston_surface_attach().
> >
> > weston_view::is_mapped affects the behaviour of weston_view_unmap() and
> > weston_view_destroy().
> >
> > When manually mapping a window of type XWAYLAND, mark both the view and
> > surface as mapped. This follows the expections in libweston, even though
> > the meaning of is_mapped is not clearly defined for either surface or
> > view.
> >
> > Also, when the XWAYLAND window is manually unmapped, unmap the
> > weston_surface. This updates weston_surface::is_mapped to reflect the
> > state. However, as a side-effect it will also unmap all sibling views,
> > should any exist.
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  libweston-desktop/xwayland.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libweston-desktop/xwayland.c b/libweston-desktop/xwayland.c
> > index 7b8f6db..6fb566a 100644
> > --- a/libweston-desktop/xwayland.c
> > +++ b/libweston-desktop/xwayland.c
> > @@ -71,6 +71,7 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
> >  					     struct weston_desktop_surface *parent,
> >  					     int32_t x, int32_t y)
> >  {
> > +	struct weston_surface *surface_base;
> >  	bool to_add = (parent == NULL && state != XWAYLAND);
> >
> >  	assert(state != NONE);
> > @@ -81,6 +82,8 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
> >  		return;
> >  	}
> >
> > +	surface_base = weston_desktop_surface_get_surface(surface->surface);  
> 
> The convention in libweston-desktop is to use "wsurface" for 
> weston_surface (and "dsurface" for weston_desktop_surface in 
> libweston-desktop shells).

Hi,

Ok.

> And why didn’t you assign the variable directly?

Makes a pretty long line. Also function calls mixed up with
declarations, even if its initializations, look messy to me: too much
happening at just declaring locals phase.

Also the value is not needed earlier, and there is a return path before
it.

> > +
> >  	if (surface->state != state) {
> >  		if (surface->state == XWAYLAND) {
> >  			assert(!surface->added);
> > @@ -88,6 +91,7 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
> >  			weston_desktop_surface_unlink_view(surface->view);
> >  			weston_view_destroy(surface->view);
> >  			surface->view = NULL;
> > +			weston_surface_unmap(surface_base);  
> 
> If we define better what “mapped” means, maybe it would make sense to 
> unmap a surface with no view?

Perhaps.

I didn't want to change the existing code more. weston_surface_unmap()
does unmap all its views, so maybe it could be done earlier. I just
didn't track through what might happen.

I've been thinking that weston_surface::mapped should reflect the
client-visible state, if mappedness has any meaning in the protocol,
while weston_view::mapped would be an internal flag. Both would roughly
mean that the object has been placed in the scene graph or at least has
a determined position and might be painted.

> Anyway, it is a good thing to do:
> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>

Nice, I'll change the variable name.


Thanks,
pq

> 
> Cheers,
> 
> 
> >  		}
> >
> >  		if (to_add) {
> > @@ -109,6 +113,8 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
> >  			weston_layer_entry_insert(&surface->xwayland->layer.view_list,
> >  						  &surface->view->layer_link);
> >  			weston_view_set_position(surface->view, x, y);
> > +			surface->view->is_mapped = true;
> > +			surface_base->is_mapped = true;
> >  		}
> >
> >  		surface->state = state;
> >  
> 
> 

-------------- 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/a81ce675/attachment.sig>