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

Fri Jul 8 19:10:53 UTC 2016

On 04.07.2016 14:14, Pekka Paalanen wrote:

Hi Pekka,

All your points below are good, so I don't think I need to reply for each one of them with "OK".

I'll implement all your suggestions and reply again if I encounter any issues with your proposals.

Thanks, Armin.

> 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: signature.asc
Type: application/pgp-signature
Size: 855 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160708/77a59a7a/attachment.sig>