[PATCH v2 1/2] shell: update shsurf->output when it might changed

[Pekka Paalanen]

On Mon,  1 Sep 2014 17:20:32 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> When we are moving window or its state changed, update the
> output, so that configure event and maximizing/fullscreening actions
> have up-to-date information.
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  desktop-shell/shell.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 26f13cc..d5996a3 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -474,6 +474,22 @@ shell_surface_state_changed(struct shell_surface *shsurf)
>  }
>  
>  static void
> +shell_surface_set_output(struct shell_surface *shsurf,
> +                         struct weston_output *output)
> +{
> +	struct weston_surface *es = shsurf->surface;
> +
> +	/* get the default output, if the client set it as NULL
> +	   check whether the ouput is available */
> +	if (output)
> +		shsurf->output = output;
> +	else if (es->output)
> +		shsurf->output = es->output;
> +	else
> +		shsurf->output = get_default_output(es->compositor);
> +}
> +
> +static void
>  shell_grab_end(struct shell_grab *grab)
>  {
>  	if (grab->shsurf) {
> @@ -484,6 +500,8 @@ shell_grab_end(struct shell_grab *grab)
>  			grab->shsurf->resize_edges = 0;
>  			shell_surface_state_changed(grab->shsurf);
>  		}
> +
> +		shell_surface_set_output(grab->shsurf, NULL);

Why does grab_end() reset the output? That looks strange. Surely if
something happened during a grab that would need shsurf->output change,
shouldn't it change there and not blindly here?

>  	}
>  
>  	weston_pointer_end_grab(grab->grab.pointer);
> @@ -2371,22 +2389,6 @@ shell_surface_set_parent(struct shell_surface *shsurf,
>  }
>  
>  static void
> -shell_surface_set_output(struct shell_surface *shsurf,
> -                         struct weston_output *output)
> -{
> -	struct weston_surface *es = shsurf->surface;
> -
> -	/* get the default output, if the client set it as NULL
> -	   check whether the ouput is available */
> -	if (output)
> -		shsurf->output = output;
> -	else if (es->output)
> -		shsurf->output = es->output;
> -	else
> -		shsurf->output = get_default_output(es->compositor);
> -}
> -
> -static void
>  surface_clear_next_states(struct shell_surface *shsurf)
>  {
>  	shsurf->next_state.maximized = false;
> @@ -5227,9 +5229,6 @@ configure(struct desktop_shell *shell, struct weston_surface *surface,
>  	if (surface->output) {
>  		wl_list_for_each(view, &surface->views, surface_link)
>  			weston_view_update_transform(view);
> -
> -		if (shsurf->state.maximized)
> -			surface->output = shsurf->output;

When you maximize a window, you want its synced-to-output to be the
output it was maximized for. Why remove this here, and what fills the
purpose now?

>  	}
>  }
>  
> @@ -5277,6 +5276,8 @@ shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>  		float from_x, from_y;
>  		float to_x, to_y;
>  
> +		shell_surface_set_output(shsurf, NULL);

Why would a client committing new content to a surface always cause the
shsurf->output to be reset?

... what is shsurf->output, anyway?

> +
>  		if (shsurf->resize_edges) {
>  			sx = 0;
>  			sy = 0;

Sorry, I just don't understand anything this patch does, and it all
looks strange to me.

I think we may also be missing some concepts here...

A wl_surface is always synced to one output, that is the
weston_surface::output. The sync is implemented in Weston core, the
repaint machinery, that sends the frame callbacks.

Then we have the complication, that there may be multiple weston_views
on different outputs, and I am unsure what their meaning wrt. to output
is.

In shell, we have shell_surface::output and
shell_surface::fullscreen_output.

I have assumed that shell_surface::output is the output that the client
says the surface should be on, like when maximizing or fullscreening on
a specific output. When the surface state becomes maximized or
fullscreened, its sync should be locked to the client specified output.

I don't know what shell_surface::output is nowadays, and why we need
fullscreen_output to be separate, or when/how those are assigned to the
weston_surface::output.

Have we lost track on what the principles here should be?

Only when a client does not explicitly specify an output, we should use
all the heuristics, but when a client has specified an output, we
should not overwrite that, in which case it should be impossible to
move the surface away from the specified output. Except then we have
the corner-case, where the surface is basically hidden, but has a
secondary view on a different output (e.g. a thumbnail), which means
the wl_surface should be temporarily synced to the outputs it is
actually on, when it is not on the intended output.

Complicated...

The patch 2/2 I can understand, but this patch I would like to hear
more justification for.


Thanks,
pq