[PATCH weston] shell: fix various interactions with the minimized state

[Pekka Paalanen]

On Thu, 31 Jul 2014 15:36:40 +0200
Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:

> This fixes the following :
> - if a surface was set fullscreen, and then minimized,
> the fullscreen compositor state would stay on and display
> a black screen ;
> - if a surface was set fullscreen, and we would then
> cycle between surfaces (with Mod+Tab e.g.), the fullscreen
> compositor state would stay on, and the fullscreen layer
> would sometimes hide surfaces positioned behind it ;
> - style and functional polishing, remove dead code.

Hi,

sounds like this should really be three separate patches, as the commit
message is a list of three things. ;-)
Or would separating produce just useless churn?

shell.c patches are hard to review to begin with, so I'd appreciate if
each patch did one thing and nothing more.

> - if a surface was set fullscreen, and we would then
> cycle between surfaces (with Mod+Tab e.g.), the fullscreen
> compositor state would stay on, and the fullscreen layer
> would sometimes hide surfaces positioned behind it ;

Hmm, are you sure this is a bug? What I see happening, when a
fullscreen surface is on top and another window is below it, and I
press Mod+tab, is that the fullscreen window does stay on top, but it
becomes almost transparent, so I can still see which window would be
raised if I released Mod+tab now. To me it looks like it is intentional
to not change the stacking order until releasing Mod+tab.

More comments below.

> 
> Signed-off-by: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> ---
>  desktop-shell/shell.c |   67 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 29 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 3c3649c..60639cc 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -2510,13 +2510,14 @@ unset_maximized(struct shell_surface *shsurf)
>  }
>  
>  static void
> -set_minimized(struct weston_surface *surface, uint32_t is_true)
> +set_minimized(struct weston_surface *surface)
>  {
> +	struct weston_view *view;
>  	struct shell_surface *shsurf;
> -	struct workspace *current_ws;
> +	struct workspace *ws;
>  	struct weston_seat *seat;
>  	struct weston_surface *focus;
> -	struct weston_view *view;
> +	float x,y;
>  
>  	view = get_default_view(surface);
>  	if (!view)
> @@ -2525,34 +2526,31 @@ set_minimized(struct weston_surface *surface, uint32_t is_true)
>  	assert(weston_surface_get_main_surface(view->surface) == view->surface);
>  
>  	shsurf = get_shell_surface(surface);
> -	current_ws = get_current_workspace(shsurf->shell);
> +	ws = get_current_workspace(shsurf->shell);
>  
> -	weston_layer_entry_remove(&view->layer_link);
> -	 /* hide or show, depending on the state */
> -	if (is_true) {
> -		weston_layer_entry_insert(&shsurf->shell->minimized_layer.view_list, &view->layer_link);
> -
> -		drop_focus_state(shsurf->shell, current_ws, view->surface);
> -		wl_list_for_each(seat, &shsurf->shell->compositor->seat_list, link) {
> -			if (!seat->keyboard)
> -				continue;
> -			focus = weston_surface_get_main_surface(seat->keyboard->focus);
> -			if (focus == view->surface)
> -				weston_keyboard_set_focus(seat->keyboard, NULL);
> -		}
> +	/* if the surface is fullscreen, unset the global fullscreen state,
> +	 * but keep the surface centered on its previous output.
> +	 */
> +	if (shsurf->state.fullscreen) {
> +		x = shsurf->view->geometry.x;
> +		y = shsurf->view->geometry.y;
> +		unset_fullscreen(shsurf);
> +		weston_view_set_position(shsurf->view, x, y);

This seems slightly strange, doesn't unset_fullscreen() cause you to
lose some bits of the fullscreen state, meaning you cannot restore it
properly later? The framerate for instance?

>  	}
> -	else {
> -		weston_layer_entry_insert(&current_ws->layer.view_list, &view->layer_link);
>  
> -		wl_list_for_each(seat, &shsurf->shell->compositor->seat_list, link) {
> -			if (!seat->keyboard)
> -				continue;
> -			activate(shsurf->shell, view->surface, seat, true);
> -		}
> +	weston_layer_entry_remove(&view->layer_link);
> +	weston_layer_entry_insert(&shsurf->shell->minimized_layer.view_list, &view->layer_link);
> +
> +	drop_focus_state(shsurf->shell, ws, view->surface);
> +	wl_list_for_each(seat, &shsurf->shell->compositor->seat_list, link) {
> +		if (!seat->keyboard)
> +			continue;
> +		focus = weston_surface_get_main_surface(seat->keyboard->focus);
> +		if (focus == view->surface)
> +			weston_keyboard_set_focus(seat->keyboard, NULL);
>  	}
>  
>  	shell_surface_update_child_surface_layers(shsurf);
> -
>  	weston_view_damage_below(view);
>  }
>  
> @@ -3534,8 +3532,7 @@ xdg_surface_set_minimized(struct wl_client *client,
>  	if (shsurf->type != SHELL_SURFACE_TOPLEVEL)
>  		return;
>  
> -	 /* apply compositor's own minimization logic (hide) */
> -	set_minimized(shsurf->surface, 1);
> +	set_minimized(shsurf->surface);
>  }
>  
>  static const struct xdg_surface_interface xdg_surface_implementation = {
> @@ -5444,12 +5441,24 @@ struct switcher {
>  static void
>  switcher_next(struct switcher *switcher)
>  {
> +	struct focus_state *state;
> +	struct weston_surface *surface;
>  	struct weston_view *view;
>  	struct weston_surface *first = NULL, *prev = NULL, *next = NULL;
>  	struct shell_surface *shsurf;
>  	struct workspace *ws = get_current_workspace(switcher->shell);
>  
> -	 /* temporary re-display minimized surfaces */
> +	/* if the focused surface is fullscreen, minimize it */
> +	wl_list_for_each(state, &ws->focus_list, link) {
> +		if (state->keyboard_focus) {
> +			surface = weston_surface_get_main_surface(state->keyboard_focus);
> +			shsurf = get_shell_surface(surface);
> +			if (shsurf->state.fullscreen)
> +				set_minimized(surface);
> +		}
> +	}

Err, can you explain what happens here and why?

Do we have a concept of a fullscreen surface being shown while not
being top-most or active? That is, is it expected to force-minimize a
fullscreen window when it is not... top-most? Regardless whether it was
minimized or not.

> +
> +	/* temporarily display minimized surfaces again */
>  	struct weston_view *tmp;
>  	struct weston_view **minimized;
>  	wl_list_for_each_safe(view, tmp, &switcher->shell->minimized_layer.view_list.link, layer_link.link) {
> @@ -5495,7 +5504,7 @@ switcher_next(struct switcher *switcher)
>  		view->alpha = 1.0;
>  
>  	shsurf = get_shell_surface(switcher->current);
> -	if (shsurf && shsurf->state.fullscreen)
> +	if (shsurf && shsurf->state.fullscreen && shsurf->fullscreen.black_view)
>  		shsurf->fullscreen.black_view->alpha = 1.0;
>  }
>  

Sorry, I'm quite confused by these parts of shell.c.


Thanks,
pq