[PATCH weston 1/2] desktop-shell: Unset fullscreen/maximized state on commit

[Jonas Ådahl]

On Tue, Aug 16, 2016 at 02:30:06PM +0200, Quentin Glidic wrote:
> From: Quentin Glidic <sardemff7+git at sardemff7.net>
> 
> Maybe libweston-desktop should prevent the committed callback to be

"Maybe" doesn't seem like something to have in a commit message :P

The reason we need it in shell_surface is that we want to know whether
we changed state from/to maximized/fullscreen, since libweston-desktop
doesn't provide a "history" of each state.

> called if no state changed. For now, keep the interesting state in
> shell_surface like they used to.
> This only stores the current state, as libweston-desktop is still in
> charge of double-buffering it.
> 
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> ---
>  desktop-shell/shell.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 7bcaa8d..cc091a9 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -128,6 +128,8 @@ struct shell_surface {
>  	struct weston_output *output;
>  
>  	struct surface_state {
> +		bool fullscreen;
> +		bool maximized;
>  		bool lowered;
>  	} state;
>  
> @@ -2404,10 +2406,10 @@ map(struct desktop_shell *shell, struct shell_surface *shsurf,
>  	struct weston_seat *seat;
>  
>  	/* initial positioning, see also configure() */
> -	if (weston_desktop_surface_get_fullscreen(shsurf->desktop_surface)) {
> +	if (shsurf->state.fullscreen) {
>  		center_on_output(shsurf->view, shsurf->fullscreen_output);
>  		shell_map_fullscreen(shsurf);
> -	} else if (weston_desktop_surface_get_maximized(shsurf->desktop_surface)) {
> +	} else if (shsurf->state.maximized) {
>  		set_maximized_position(shell, shsurf);
>  	} else {
>  		weston_view_set_initial_position(shsurf->view, shell);
> @@ -2418,7 +2420,7 @@ map(struct desktop_shell *shell, struct shell_surface *shsurf,
>  
>  	weston_view_update_transform(shsurf->view);
>  	shsurf->view->is_mapped = true;
> -	if (weston_desktop_surface_get_maximized(shsurf->desktop_surface)) {
> +	if (shsurf->state.maximized) {
>  		surface->output = shsurf->output;
>  		shsurf->view->output = shsurf->output;
>  	}
> @@ -2429,8 +2431,7 @@ map(struct desktop_shell *shell, struct shell_surface *shsurf,
>  				 WESTON_ACTIVATE_FLAG_CONFIGURE);
>  	}
>  
> -	if (!weston_desktop_surface_get_maximized(shsurf->desktop_surface) &&
> -	    !weston_desktop_surface_get_fullscreen(shsurf->desktop_surface)) {
> +	if (!shsurf->state.fullscreen && !shsurf->state.maximized) {
>  		switch (shell->win_animation_type) {
>  		case ANIMATION_FADE:
>  			weston_fade_run(shsurf->view, 0.0, 1.0, 300.0, NULL, NULL);
> @@ -2455,10 +2456,23 @@ desktop_surface_committed(struct weston_desktop_surface *desktop_surface,
>  		weston_desktop_surface_get_surface(desktop_surface);
>  	struct weston_view *view = shsurf->view;
>  	struct desktop_shell *shell = data;
> +	bool old_fullscreen;
> +	bool old_maximized;

nit: better names: was_fullscreen, was_maximized;

>  
>  	if (surface->width == 0)
>  		return;
>  
> +	if (shsurf->state.fullscreen)
> +		unset_fullscreen(shsurf);
> +	if (shsurf->state.maximized)
> +		unset_maximized(shsurf);

This doesn't look right. I'd rather see you continued only "unsetting"
things if they were actually unset. See below.

> +
> +	old_fullscreen = shsurf->state.fullscreen;
> +	old_maximized = shsurf->state.maximized;
> +
> +	shsurf->state.fullscreen = weston_desktop_surface_get_fullscreen(desktop_surface);
> +	shsurf->state.maximized = weston_desktop_surface_get_maximized(desktop_surface);

nit: These are both longer than 80 chars.

> +
>  	if (!weston_surface_is_mapped(surface)) {
>  		map(shell, shsurf, sx, sy);
>  		surface->is_mapped = true;
> @@ -2469,12 +2483,14 @@ desktop_surface_committed(struct weston_desktop_surface *desktop_surface,
>  
>  	if (sx == 0 && sy == 0 &&
>  	    shsurf->last_width == surface->width &&
> -	    shsurf->last_height == surface->height)
> +	    shsurf->last_height == surface->height &&
> +	    old_fullscreen == shsurf->state.fullscreen &&
> +	    old_maximized == shsurf->state.maximized)
>  	    return;
>  
> -	if (weston_desktop_surface_get_fullscreen(desktop_surface))
> +	if (shsurf->state.fullscreen) {

You can change this to

	if (!was_fullscreen && shsurf->state.fullscreen) {
		shell_configure_fullscreen(shsurf);
	} else if (was_fullscreen && !shsurf->state.fullscreen) {
		unset_fullscreen(shsurf);
	}

	if (!was_maximized && shsurf->state.maximized) {
		set_maximized_position(shell, shsurf);
		surface->output = shsurf->output;
	} else if (was_maximized && !shsurf->stat.maximized) {
		unset_maximized(shsurf);
	}

since its always the shell that requests the state (i.e. if we get both
fullscreen and maximize at the same time its a programming error in the
shell, meaning we can do something like

	assert((!shsurf.state.fullscreen && !shsurf.state.maximized) ||
	       shsurf.state.fullscreen != shsurf.state.maximized);

in the beginning of the function to verify that we never configured an
invalid state.


Jonas

>  		shell_configure_fullscreen(shsurf);
> -	else if (weston_desktop_surface_get_maximized(desktop_surface)) {
> +	} else if (shsurf->state.maximized) {
>  		set_maximized_position(shell, shsurf);
>  		surface->output = shsurf->output;
>  	} else {
> @@ -2531,8 +2547,6 @@ set_fullscreen(struct shell_surface *shsurf, bool fullscreen,
>  
>  		width = shsurf->output->width;
>  		height = shsurf->output->height;
> -	} else {
> -		unset_fullscreen(shsurf);
>  	}
>  	weston_desktop_surface_set_fullscreen(desktop_surface, fullscreen);
>  	weston_desktop_surface_set_size(desktop_surface, width, height);
> @@ -2632,8 +2646,6 @@ set_maximized(struct shell_surface *shsurf, bool maximized)
>  
>  		width = area.width;
>  		height = area.height;
> -	} else {
> -		unset_maximized(shsurf);
>  	}
>  	weston_desktop_surface_set_maximized(desktop_surface, maximized);
>  	weston_desktop_surface_set_size(desktop_surface, width, height);
> -- 
> 2.9.2
>