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

[Quentin Glidic]

On 19/08/2016 11:55, Jonas Ådahl wrote:
> 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.

If libweston-desktop only calls this function when something changed, 
you don’t need an history on this side, nor an access to the 
libweston-desktop history.
The commit message is an idea for a future improvement.

Maybe I should try to do it now.


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

Or just move them after the “something changed” check?


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

Shouldn’t we make that a part of the promise of libweston-desktop? That 
means adding an assert that you cannot call 
weston_desktop_surface_set_fullscreen() and _set_maximized() both with true.
And minimized too, I guess?

What if a user hit the maximize button, then the fullscreen button, then 
the fullscreen button again. Should the surface be maximized or not? 
Should this be history, or just accumulation of states with one 
overriding the other?


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


-- 

Quentin “Sardem FF7” Glidic