[weston PATCH v7 1/2] add the set_maximised implementation

[Pekka Paalanen]

Hi,

comments inline.

On Tue,  7 Feb 2012 11:42:30 +0800
juan.j.zhao at linux.intel.com wrote:

> From: Juan Zhao <juan.j.zhao at linux.intel.com>
> 
> ---
>  src/shell.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/src/shell.c b/src/shell.c
> index d15c8e2..e6aeeae 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -81,6 +81,7 @@ enum shell_surface_type {
>  	SHELL_SURFACE_TOPLEVEL,
>  	SHELL_SURFACE_TRANSIENT,
>  	SHELL_SURFACE_FULLSCREEN,
> +	SHELL_SURFACE_MAXIMISED,
>  	SHELL_SURFACE_POPUP
>  };
>  
> @@ -365,6 +366,7 @@ reset_shell_surface_type(struct shell_surface *surface)
>  	case SHELL_SURFACE_TOPLEVEL:
>  	case SHELL_SURFACE_TRANSIENT:
>  	case SHELL_SURFACE_POPUP:
> +	case SHELL_SURFACE_MAXIMISED:
>  		break;
>  	}
>  
> @@ -418,6 +420,66 @@ get_default_output(struct weston_compositor *compositor)
>  			    struct weston_output, link);
>  }
>  
> +static struct wl_shell *
> +shell_surface_get_shell(struct shell_surface *shsurf)
> +{
> +	struct weston_surface *es = shsurf->surface;
> +	struct weston_shell *shell=es->compositor->shell;
> +
> +	return (struct wl_shell *)container_of(shell, struct wl_shell, shell);
> +}
> +
> +static int
> +get_output_panel_height(struct wl_shell *wlshell,struct weston_output *output)
> +{
> +	struct shell_surface *priv;
> +	int panel_height = 0;
> +
> +	if(!output)
> +		return 0;
> +
> +	wl_list_for_each(priv, &wlshell->panels, link) {
> +		if (priv->output == output) {
> +			panel_height = priv->surface->geometry.height;
> +			break;
> +		}
> +	}
> +	return panel_height;
> +}
> +
> +static void
> +shell_surface_set_maximised(struct wl_client *client,
> +			    struct wl_resource *resource,
> +			    struct wl_resource *output_resource )
> +{
> +	struct shell_surface *shsurf = resource->data;
> +	struct weston_surface *es = shsurf->surface;
> +	struct weston_output *output = NULL;
> +	struct wl_shell *wlshell = NULL;
> +	uint32_t edges = 0, panel_height = 0;
> +
> +	if (reset_shell_surface_type(shsurf) || !output_resource)

As reset_shell_surface_type() will modify the surface state, you should
do that after all other checks. Resetting the type and then aborting
this operation for another reason will leave us in inconsistent state
in what the client and the server think about the surface.

Either maximisation operation succeeds, or the client is notified
somehow it failed. If the client is not notified, it assumes the
operation succeeded, and e.g. draws the window decorations according to
maximised state.

> +		return;
> +
> +	if ( es->output != output_resource->data )
> +		return;

I do not think it was the intent to simply refuse to maximise if the
supplied output is not the one the surface is currently on. This also
prevents maximising surfaces that have not been mapped yet.

If given an output, that output should be used, even if the current
output for the surface is different. Otherwise adding the output
parameter to this method is meaningless, since you can't do anything
with it.

> +
> +	shsurf->saved_x = es->geometry.x;
> +	shsurf->saved_y = es->geometry.y;
> +
> +	wlshell = shell_surface_get_shell(shsurf);
> +	panel_height = get_output_panel_height(wlshell, es->output);
> +
> +	edges = WL_SHELL_SURFACE_RESIZE_TOP|WL_SHELL_SURFACE_RESIZE_LEFT;
> +
> +	wl_resource_post_event(&shsurf->resource,
> +			       WL_SHELL_SURFACE_CONFIGURE,
> +			       weston_compositor_get_time(), edges,
> +			       es->output->current->width, es->output->current->height-panel_height);
> +	weston_surface_damage(es);

Why damaging the surface here? Nothing has changed with respect to what
should be rendered on screen.

> +	shsurf->type = SHELL_SURFACE_MAXIMISED;
> +}
> +
>  static void
>  shell_surface_set_fullscreen(struct wl_client *client,
>  			     struct wl_resource *resource)
> @@ -557,6 +619,7 @@ static const struct wl_shell_surface_interface shell_surface_implementation = {
>  	shell_surface_set_toplevel,
>  	shell_surface_set_transient,
>  	shell_surface_set_fullscreen,
> +	shell_surface_set_maximised,
>  	shell_surface_set_popup
>  };
>  
> @@ -1305,6 +1368,11 @@ map(struct weston_shell *base,
>  	case SHELL_SURFACE_FULLSCREEN:
>  		center_on_output(surface, surface->fullscreen_output);
>  		break;
> +	case SHELL_SURFACE_MAXIMISED:
> +		surface->geometry.x = 0;
> +		surface->geometry.y = get_output_panel_height(shell,surface->output);
> +		surface->geometry.dirty = 1;
> +		break;
>  	case SHELL_SURFACE_LOCK:
>  		center_on_output(surface, get_default_output(compositor));
>  		break;
> @@ -1381,6 +1449,7 @@ map(struct weston_shell *base,
>  	case SHELL_SURFACE_TOPLEVEL:
>  	case SHELL_SURFACE_TRANSIENT:
>  	case SHELL_SURFACE_FULLSCREEN:
> +	case SHELL_SURFACE_MAXIMISED:
>  		if (!shell->locked)
>  			activate(base, surface,
>  				 (struct weston_input_device *)
> @@ -1419,6 +1488,10 @@ configure(struct weston_shell *base, struct weston_surface *surface,
>  	case SHELL_SURFACE_FULLSCREEN:
>  		center_on_output(surface, surface->fullscreen_output);
>  		break;
> +	case SHELL_SURFACE_MAXIMISED:
> +		x = 0;
> +		y = get_output_panel_height(shell,surface->output);
> +		break;
>  	default:
>  		break;
>  	}

Thanks,
pq