[PATCH 2/3] shell: Add implementation of fullscreen.

[wuzhiwen]

Hi khr, 
Comments inlined.

>-----Original Message-----
>From:
>wayland-devel-bounces+zhiwen.wu=linux.intel.com at lists.freedesktop.org
>[mailto:wayland-devel-bounces+zhiwen.wu=linux.intel.com at lists.freedesktop.o
>rg] On Behalf Of Kristian Hoegsberg
>Sent: Monday, February 27, 2012 4:03 AM
>To: zhiwen.wu at linux.intel.com
>Cc: juan.j.zhao at linux.intel.com; krh at bitplanet.net; ppaalanen at gmail.com;
>wayland-devel at lists.freedesktop.org
>Subject: Re: [PATCH 2/3] shell: Add implementation of fullscreen.
>
>On Sun, Feb 26, 2012 at 03:21:37PM +0800, zhiwen.wu at linux.intel.com wrote:
>> From: Alex Wu <zhiwen.wu at linux.intel.com>
>
>Nice.  There are a few issues below and one thing I didn't figure out right
now
>was that when you raise a window over the fullscreen window and then
re-raise
>the fullscreen window, the black surface renders partly transparent (I can
see
>the panel through it, but it doesn't receive input).
[Wu, Zhiwen] I can't reproduce the " the black surface renders partly
transparent " in my laptop.
           But, I find another issues by this test case: re-raise the
fullscreen window only raise the black surface.
           I will fix it in next reversion. Thanks for your testing.
>
>> ---
>>  src/shell.c |  207
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 files changed, 191 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/shell.c b/src/shell.c index ee71dcc..641a53f 100644
>> --- a/src/shell.c
>> +++ b/src/shell.c
>> @@ -108,6 +108,14 @@ struct shell_surface {
>>  		int32_t initial_up;
>>  	} popup;
>>
>> +	struct {
>> +		enum wl_shell_surface_fullscreen_method type;
>> +		struct weston_transform transform; /* matrix from x, y */
>> +		bool setted;/*If transform already added to the list*/
>
>Can we call it transform_set?  Or maybe just use
>wl_list_empty(&transform.link) to indicate it's not set?  That means you
have
>to wl_list_init() it when you take it out of the list.
>

[Wu, Zhiwen] Yes, will fix in next reversion.

>> +		uint32_t framerate;
>> +		struct weston_surface *black_surface;
>> +	} fullscreen;
>> +
>>  	struct weston_output *fullscreen_output;
>>  	struct weston_output *output;
>>  	struct wl_list link;
>> @@ -130,6 +138,14 @@ struct rotate_grab {  };
>>
>>  static void
>> +activate(struct weston_shell *base, struct weston_surface *es,
>> +	 struct weston_input_device *device, uint32_t time);
>> +
>> +static void
>> +center_on_output(struct weston_surface *surface,
>> +		 struct weston_output *output);
>> +
>> +static void
>>  shell_configuration(struct wl_shell *shell)  {
>>  	char *config_file;
>> @@ -299,7 +315,8 @@ weston_surface_resize(struct shell_surface
>> *shsurf,  {
>>  	struct weston_resize_grab *resize;
>>
>> -	/* FIXME: Reject if fullscreen */
>> +	if (shsurf->type == SHELL_SURFACE_FULLSCREEN)
>> +		return 0;
>>
>>  	if (edges == 0 || edges > 15 ||
>>  	    (edges & 3) == 3 || (edges & 12) == 12) @@ -330,7 +347,8 @@
>> shell_surface_resize(struct wl_client *client, struct wl_resource
*resource,
>>  	struct weston_input_device *wd = input_resource->data;
>>  	struct shell_surface *shsurf = resource->data;
>>
>> -	/* FIXME: Reject if fullscreen */
>> +	if (shsurf->type == SHELL_SURFACE_FULLSCREEN)
>> +		return;
>>
>>  	if (wd->input_device.button_count == 0 ||
>>  	    wd->input_device.grab_time != time || @@ -348,15 +366,32 @@
>> get_default_output(struct weston_compositor *compositor)
>>  			    struct weston_output, link);
>>  }
>>
>> +static void
>> +shell_unmap_fullscreen(struct shell_surface *shsurf) {
>> +	weston_surface_set_position(shsurf->surface,
>> +			shsurf->saved_x,
>> +			shsurf->saved_y);
>> +	shsurf->fullscreen_output = NULL;
>> +	shsurf->fullscreen.type = 0;
>> +	weston_surface_unmap(shsurf->fullscreen.black_surface);
>> +	wl_list_init(&shsurf->fullscreen.black_surface->link);
>
>We can just destroy the black surface here.
>
>> +	weston_surface_unmap(shsurf->surface);
>> +	wl_list_init(&shsurf->surface->link);
>> +
>> +	if (shsurf->fullscreen.setted) {
>> +		wl_list_remove(&shsurf->fullscreen.transform.link);
>> +		wl_list_init(&shsurf->fullscreen.transform.link);
>> +		shsurf->fullscreen.setted = false;
>> +	}
>> +}
>> +
>>  static int
>>  reset_shell_surface_type(struct shell_surface *surface)  {
>>  	switch (surface->type) {
>>  	case SHELL_SURFACE_FULLSCREEN:
>> -		weston_surface_set_position(surface->surface,
>> -					    surface->saved_x,
>> -					    surface->saved_y);
>> -		surface->fullscreen_output = NULL;
>> +		shell_unmap_fullscreen(surface);
>
>We don't want a full unmap here.  One problem is that when you go from
>fullscreen back to toplevel, the window gets remapped including picking a
new
>random initial position and triggering the zoom+fade in animation.
[Wu, Zhiwen] Yes for now. Actually, I didn't figure out a perfect solution
for this case.
           A possible solution is just unmap the black surface, and
re-stacking the fullscreen to below the panels in this func?

>>  shell_surface_set_fullscreen(struct wl_client *client,
>>  			     struct wl_resource *resource, @@ -492,21
+633,37 @@
>> shell_surface_set_fullscreen(struct wl_client *client,  {
>>  	struct shell_surface *shsurf = resource->data;
>>  	struct weston_surface *es = shsurf->surface;
>> -	struct weston_output *output;
>> +	struct wl_shell *shell;
>> +
>> +	if (output_resource)
>> +		shsurf->output = output_resource->data;
>> +	else
>> +		shsurf->output = get_default_output(es->compositor);
>>
>>  	if (reset_shell_surface_type(shsurf))
>>  		return;
>>
>> -	/* FIXME: Fullscreen on first output */
>> -	/* FIXME: Handle output going away */
>> -	output = get_default_output(es->compositor);
>> -
>>  	shsurf->saved_x = es->geometry.x;
>>  	shsurf->saved_y = es->geometry.y;
>> -	shsurf->output = output;
>> -	shsurf->fullscreen_output = output;
>> +	shsurf->fullscreen_output = shsurf->output;
>> +	shsurf->fullscreen.type = method;
>> +	shsurf->fullscreen.framerate = framerate;
>>  	shsurf->type = SHELL_SURFACE_FULLSCREEN;
>>
>> +	if (shsurf->fullscreen.black_surface == NULL)
>> +		shsurf->fullscreen.black_surface =
>> +create_black_surface(es->compositor);
>> +
>> +	if (es->output) {
>
>This is never true since reset_shell_surface_type() unmaps it.  But even
when
>that is fixed, we can't change the surface position, size or stacking until
we
>receive the fullscreen buffer from the client.  If we do, we're very likely
to see a
>frame or two with the surface jumping or moving before we get the real
>fullscreen buffer. All we should do here is save the fullscreen parameters
and
>send the configure.  Once we get the new buffer in configure, we can do all
the
>changes (create black surface, raise, move, add transform etc).
>
[Wu, Zhiwen] 
these are 2 cases that we have no chance to call shell.c::configure():
1. no new buffer (e.g. client ignored the configure event)
2. new buffer attached, but buffer size is the same as previous one(e.g. the
client buffer size is the same as output's).
I am thinking of the solution that we can specify in the protocol that if
client choose to "scale" method, that means client will ignore the configure
event.
So that, shell will do all the fullscreen stuff in this func if es->output
!= NULL. For the other methods, if current buffer size is the same as
output, do the fullscreen stuff immediately, if not, leave it to configure.
The Pseudocode:
If (es->output && (method == WL_SHELL_SURFACE_FULLSCREEN_METHOD_SCALE ||
current buffer size == output size)) {
		//do the fullscreen stuff.
}