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

[wuzhiwen]

Hi Pekka,
Good jumping in:), that really fix my concerns on configure.

>-----Original Message-----
>From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
>Sent: Monday, February 27, 2012 3:50 PM
>To: wuzhiwen
>Cc: 'Kristian Hoegsberg'; juan.j.zhao at linux.intel.com; krh at bitplanet.net;
>wayland-devel at lists.freedesktop.org
>Subject: Re: [PATCH 2/3] shell: Add implementation of fullscreen.
>
>Sorry for jumping in...
>
>On Mon, 27 Feb 2012 14:25:23 +0800
>"wuzhiwen" <zhiwen.wu at linux.intel.com> wrote:
>
>> >> 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)
>
>In that case the client is misbehaving. IIRC clients must respond to the
latest
>configure event, this is a general core principle.
>
>If the client does not send a new buffer, a user cannot see if it sent a
>set_* request either.
>
>> 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).
>
>Ah, I see the problem. I would propose adding a flag to struct
weston_surface
>for "force configure", that will override the position change and size
check and
>always call configure() on the next attach (but only if the next attach
didn't
>trigger map()).
>
>> 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.
>
>Adding special cases like that to the protocol to circumvent some
>implementation detail problem is nasty, and will lead to unmaintainable and
>difficult protocol. NAK, IMO. Let's put an effort to keep the protocol as
simple as
>possible, and preferrably void of any exceptions to the general rules.
>
>> 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.
>> }
>
>With the "force configure" flag, we don't need this special case behaviour.
Would
>you agree, or do you see further problems?
>I known I'm just hand-waving here.
>
>
>Thanks,
>pq