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

[Pekka Paalanen]

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