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

[Kristian Høgsberg]

On Mon, Feb 27, 2012 at 3:33 AM, wuzhiwen <zhiwen.wu at linux.intel.com> wrote:
> Hi Pekka,
> Good jumping in:), that really fix my concerns on configure.

Yeah, thanks Pekka, that's basically what I was going to say :)

Kristian

>>-----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
>