[PATCH] fullscreen-shell: Remap surface when attaching primary output

[Pekka Paalanen]

On Thu, 30 Jun 2016 19:55:01 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> When a surface gets created and no outputs are present,
> it won't get properly configured. This happens because
> call to configure_presented_surface() depends on a call
> to fs_output_set_surface() which requires a valid output.
> 
> This patch makes the code save the surface when no
> outputs are present and reconfigure it when an output
> gets attached, so it gets properly displayed.
> 
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
>  fullscreen-shell/fullscreen-shell.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/fullscreen-shell/fullscreen-shell.c b/fullscreen-shell/fullscreen-shell.c
> index 9716dc5..f58df2f 100644
> --- a/fullscreen-shell/fullscreen-shell.c
> +++ b/fullscreen-shell/fullscreen-shell.c
> @@ -46,6 +46,8 @@ struct fullscreen_shell {
>  	struct wl_listener output_created_listener;
>  
>  	struct wl_listener seat_created_listener;
> +
> +	struct wl_list unmapped_surfaces;

Hi Armin,

this member should have a comment saying what link element it contains.

You mentioned that you would have another patch for handling the case
where a client uses present_surface with a NULL output first, and then
the final output get unplugged and re-plugged. Would it make sense if
this was a list of surfaces presented with NULL output instead?

You'd go through those surfaces every time an output gets plugged in,
to create a copy of the view on that output. I suppose that case is not
implemented at all yet in upstream?

Might be better to have both patches in the same series, since I think
they could share a lot.

>  };
>  
>  struct fs_output {
> @@ -83,6 +85,12 @@ struct pointer_focus_listener {
>  	struct wl_listener seat_destroyed;
>  };
>  
> +struct fs_surface_list {

More like an item than a list, yes?

> +	struct weston_surface *surface;

This member needs to be accompanied with a destroy listener, I believe.
A client request (wl_surface.destroy) may cause weston_surface to be
destroyed, and you do not want to be left with a stale pointer.

> +	enum zwp_fullscreen_shell_v1_present_method method;
> +	struct wl_list link;
> +};
> +
>  static void
>  pointer_focus_changed(struct wl_listener *listener, void *data)
>  {
> @@ -245,10 +253,15 @@ pending_surface_destroyed(struct wl_listener *listener, void *data)
>  	fsout->pending.surface = NULL;
>  }
>  
> +static void
> +configure_presented_surface(struct weston_surface *surface, int32_t sx,
> +			    int32_t sy);
> +
>  static struct fs_output *
>  fs_output_create(struct fullscreen_shell *shell, struct weston_output *output)
>  {
>  	struct fs_output *fsout;
> +	struct fs_surface_list *surf, *next;
>  
>  	fsout = zalloc(sizeof *fsout);
>  	if (!fsout)
> @@ -271,6 +284,14 @@ fs_output_create(struct fullscreen_shell *shell, struct weston_output *output)
>  	weston_layer_entry_insert(&shell->layer.view_list,
>  		       &fsout->black_view->layer_link);
>  	wl_list_init(&fsout->transform.link);
> +
> +	wl_list_for_each_safe(surf, next, &shell->unmapped_surfaces, link) {
> +		fs_output_set_surface(fsout, surf->surface, surf->method, 0, 0);

This actually shows that unmapped_surfaces can only ever contain one
surface, or at least that is what makes sense for this piece of code.

If fullscreen-shell was written to support the client switching like
the protocol spec defines, you could have at most one surface for the
NULL-output per client. It doesn't, but we can still keep the list as
you wrote it, since that is probably how someone adding multi-client
support would do it.

To be really correct, you'd have to check the surface is from the right
client. You could just add a XXX comment, like below.

> +		configure_presented_surface(surf->surface, 0, 0);
> +		wl_list_remove(&surf->link);

If the list gathered the surfaces presented for a NULL-output, you
wouldn't remove the link here.

> +		free(surf);

Otherwise the code looks fine for what you meant it to do.

> +	}
> +
>  	return fsout;
>  }
>  
> @@ -676,6 +697,7 @@ fullscreen_shell_present_surface(struct wl_client *client,
>  	struct weston_surface *surface;
>  	struct weston_seat *seat;
>  	struct fs_output *fsout;
> +	struct fs_surface_list *surf;
>  
>  	surface = surface_res ? wl_resource_get_user_data(surface_res) : NULL;
>  
> @@ -692,7 +714,13 @@ fullscreen_shell_present_surface(struct wl_client *client,
>  				       "Invalid presentation method");
>  	}
>  
> -	if (output_res) {
> +	if (wl_list_empty(&shell->output_list)) {

I think this should happen only if output_res is NULL.

If output_res is not NULL, then the client thinks the output still
exists. If our output list is empty however, the client just has not
had the chance to process the wl_registry.global_remove event yet. The
client should process it soon, and when it does, it should fix the
situation with the wl_surface on its own.

If output_res is not NULL, the surface is not for the "show everywhere"
case, but for a specific output. Once a global has been removed, it
cannot come back, so the wl_output will be permanently inert too. It
doesn't matter if the same monitor gets plugged back to the same
connector, we don't maintain that association at the protocol level.

> +		surf = zalloc(sizeof *surf);
> +		surf->surface = surface;
> +		surf->method = method;
> +		wl_list_init(&surf->link);

Init not needed, wl_list_insert() overwrites it anyway.

> +		wl_list_insert(shell->unmapped_surfaces.prev, &surf->link);

Here it is possible that several surfaces end up in the list. However,
only the latest one should stay there per client.

Fullscreen-shell does not support multiple clients atm. anyway, so just
having a XXX comment would be enough.

> +	} else if (output_res) {
>  		output = wl_resource_get_user_data(output_res);
>  		fsout = fs_output_for_output(output);
>  		fs_output_set_surface(fsout, surface, method, 0, 0);
> @@ -831,6 +859,7 @@ module_init(struct weston_compositor *compositor,
>  		return -1;
>  
>  	shell->compositor = compositor;
> +	wl_list_init(&shell->unmapped_surfaces);
>  
>  	shell->client_destroyed.notify = client_destroyed;
>  

Essentially a good patch with some details to fix, but I'd like to see
the second use case addressed in the same series too.


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