[PATCH 6/7] fullscreen-shell: Remap surfaces when attaching a primary output

[Pekka Paalanen]

On Fri, 29 Jul 2016 13:26:27 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> When no outputs are present, and no output resource was given,
> a fullscreen surface won't get configured because the default
> behavior when there is no output resource given is to create
> the same surface on all fullscreen outputs.
> 
> This patch makes the shell reconfigure the surface as soon
> as an output gets plugged in and configured, so it can properly
> be displayed if it was started when no outputs were present.
> 
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
>  fullscreen-shell/fullscreen-shell.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Hi Armin,

the previous patch should have been squashed with this patch. The
review comments here are partially for the previous patch too.

> 
> diff --git a/fullscreen-shell/fullscreen-shell.c b/fullscreen-shell/fullscreen-shell.c
> index 3dbd0d9..71d609c 100644
> --- a/fullscreen-shell/fullscreen-shell.c
> +++ b/fullscreen-shell/fullscreen-shell.c
> @@ -299,10 +299,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_client_surface *surf, *next;
>  
>  	fsout = zalloc(sizeof *fsout);
>  	if (!fsout)
> @@ -325,6 +330,13 @@ 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);
> +		configure_presented_surface(surf->surface, 0, 0);
> +		remove_unmapped_surface(surf);
> +	}

I think this hunk needs an XXX comment too. Right now the code says:
when a new output is created, assing all not yet mapped surfaces to it.
It doesn't make sense because an output can only have one surface at a
time. The comment should explain why the code is saying strange things.

Or better, fix the code to make sense, e.g. search the list for one
particular surface intended for this particular output.

Hmm, no, that does not make sense either. There cannot be a surface
dedicated for this output, because the output has just been created.
The client has not had a chance to create and assign a surface.

So, this is to handle the surfaces assigned to no-particular-output
instead? For which there can be only one, as there can be only one
client.

In that case, the list should be documented to contain the surfaces
that were assigned to the NULL-output, i.e. no-particular-output. It's
not a list of just any unmapped surfaces, it is a list of surfaces
presented to no-particular-output. Also the list name should reflect
that.

Then I think it would make better sense, but this hunk of code still
needs the XXX comment.

The remove_unmapped_surface() call does not seem right either. The
surface should stay in the list until another surface is presented for
output NULL. If you look at...

> +
>  	return fsout;
>  }
>  
> @@ -750,6 +762,8 @@ fullscreen_shell_present_surface(struct wl_client *client,
>  		output = wl_resource_get_user_data(output_res);
>  		fsout = fs_output_for_output(output);
>  		fs_output_set_surface(fsout, surface, method, 0, 0);
> +	} else if (wl_list_empty(&shell->output_list)) {
> +		add_unmapped_surface(shell, surface, method);
>  	} else {
>  		wl_list_for_each(fsout, &shell->output_list, link)
>  			fs_output_set_surface(fsout, surface, method, 0, 0);

...this wl_list_for_each() loop, it seems it should be possible to set
the same surface to several outputs. Keeping the surface in the
"presented for the NULL output" list would allow additional outputs to
get a clone of the surface later too.

The condition on when to call add_unmapped_surface() looks wrong. It
should be called if and only if output_res is NULL.

Whether the output list is currently empty or not does not actually
matter at all. A surface presented for NULL output is what it is, and
should be kept in the surface list until replaced or destroyed. Every
time a new output gets plugged in, the surface list needs to be
processed so that the surface gets cloned on the new output.

I think the code you wrote works for the single output case, but it
does not generalize for more outputs, nor it handles replacing the
surface presented for the NULL output.


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/20160810/09c8a3e4/attachment.sig>