[PATCH weston 06/11] shell: Organize workspace containers by output

[Pekka Paalanen]

Hi Jonas,

this raised a couple of thoughts, inline below.


On Sat, 26 Jan 2013 15:33:36 +0100
Jonas Ådahl <jadahl at gmail.com> wrote:

> Instead of having one global workspace container, have one container per
> output. This means that every output will have its own set of workspaces
> with its own workspace state.
> 
> New surfaces are added to the current workspace on the container of the
> output the surface was positioned at.
> 
> The workspace manager interface is currently limited to only represent
> the container of the first output. All events and requests are handleded
> as if there was only one container.
> 
> The keyboard bindings for changing workspaces are updated to change the
> state of the container of the output where the pointer is on. If no
> pointer exists, the workspace changing bindings are no-ops.
> 
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>  src/shell.c |  367 +++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 266 insertions(+), 101 deletions(-)
> 
> diff --git a/src/shell.c b/src/shell.c
> index c5bb9c4..45e70f0 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -36,6 +36,7 @@
>  #include "compositor.h"
>  #include "desktop-shell-server-protocol.h"
>  #include "workspaces-server-protocol.h"
> +#include "../shared/hash.h"
>  #include "../shared/config-parser.h"
>  
>  #define DEFAULT_NUM_WORKSPACES 1
> @@ -62,6 +63,8 @@ struct workspace_container {
>  	unsigned int current;
>  	unsigned int num;
>  
> +	struct weston_output *output;
> +
>  	struct weston_animation animation;
>  	struct wl_list anim_sticky_list;
>  	int anim_dir;
> @@ -120,7 +123,7 @@ struct desktop_shell {
>  	struct wl_listener lock_surface_listener;
>  
>  	struct {
> -		struct workspace_container container;
> +		struct hash_table *table;
>  		unsigned int default_num;
>  
>  		struct wl_list client_list;
> @@ -234,9 +237,6 @@ static void
>  activate(struct desktop_shell *shell, struct weston_surface *es,
>  	 struct weston_seat *seat);
>  
> -static struct workspace *
> -get_current_workspace(struct desktop_shell *shell);
> -
>  static struct shell_surface *
>  get_shell_surface(struct weston_surface *surface);
>  
> @@ -576,50 +576,98 @@ workspace_restack_surface(struct workspace *ws, struct shell_surface *shsurf)
>  }
>  
>  static struct workspace *
> -get_workspace(struct desktop_shell *shell, unsigned int index)
> +workspace_container_get(struct workspace_container *container,
> +			unsigned int index)
>  {
> -	struct workspace **pws = shell->workspaces.container.workspaces.data;
> -	assert(index < shell->workspaces.container.num);
> -	pws += index;
> -	return *pws;
> +	struct workspace **pws = container->workspaces.data;
> +	return *(pws + index);
>  }
>  
>  static struct workspace *
> -get_current_workspace(struct desktop_shell *shell)
> +workspace_container_get_current(struct workspace_container *container)
>  {
> -	return get_workspace(shell, shell->workspaces.container.current);
> +	return workspace_container_get(container, container->current);
>  }
>  
> -static void
> -activate_workspace(struct desktop_shell *shell, unsigned int index)
> +static struct workspace_container *
> +find_first_workspace_container(struct desktop_shell *shell)
>  {
> -	struct workspace *ws;
> +	struct weston_output *output;
> +	struct workspace_container *container;
>  
> -	ws = get_workspace(shell, index);
> -	wl_list_insert(&shell->panel_layer.link, &ws->layer.link);
> +	output = container_of(shell->compositor->output_list.next,
> +			      struct weston_output, link);

I'd like to have an assert added with all code that assumes we have at
least one output. There will be a time when we want to be able to deal
with a temporary zero outputs cases, and having asserts around will make
finding all places that need fixing easier. Or something to the same
effect.

Hmm, or maybe just grepping for output_list with a container_of is
enough.

> +	container = hash_table_lookup(shell->workspaces.table, output->id);
> +
> +	return container;
> +}
> +
> +static struct workspace_container *
> +find_focused_workspace_container(struct desktop_shell *shell,
> +				 struct wl_seat *seat)
> +{
> +	int x, y;
> +	struct weston_output *output;
> +
> +	if (seat->pointer == NULL)
> +		return NULL;
> +
> +	x = wl_fixed_to_int(seat->pointer->x);
> +	y = wl_fixed_to_int(seat->pointer->y);
> +	wl_list_for_each(output, &shell->compositor->output_list, link)
> +		if (pixman_region32_contains_point(&output->region,
> +						   x, y, NULL))
> +			return hash_table_lookup(shell->workspaces.table,
> +						 output->id);
>  
> -	shell->workspaces.container.current = index;
> +	return NULL;
> +}
> +
> +static struct workspace_container *
> +get_workspace_container(struct desktop_shell *shell, uint32_t container_id)
> +{
> +	return hash_table_lookup(shell->workspaces.table, container_id);
> +}
> +
> +static struct workspace *
> +find_workspace_for(struct desktop_shell *shell, struct weston_surface *surface)
> +{
> +	struct workspace_container *container;
> +
> +	if (surface->output)
> +		container = get_workspace_container(shell, surface->output->id);
> +	else
> +		container = find_first_workspace_container(shell);
> +	return workspace_container_get_current(container);
>  }
>  
>  static void
> -workspace_container_release(struct workspace_container *container)
> +workspace_container_destroy(struct workspace_container *container)
>  {
>  	struct workspace **pws;
>  
>  	wl_array_for_each(pws, &container->workspaces)
>  		workspace_destroy(*pws);
>  	wl_array_release(&container->workspaces);
> +	free(container);
>  }
>  
> -static int
> -workspace_container_init(struct workspace_container *container,
> -			 struct desktop_shell *shell)
> +static struct workspace_container *
> +workspace_container_create(struct desktop_shell *shell,
> +			   struct weston_output *output,
> +			   unsigned int num)
>  {
> +	struct workspace_container *container;
>  	struct workspace **pws;
>  	unsigned int i;
>  
> +	container = malloc(sizeof *container);
> +	if (container == NULL)
> +		return NULL;
> +
> +	container->output = output;

I think we will be needing a destroy signal for outputs, and you could
hook into it here - do you want to dynamically remove the workspace
when the output goes away?

Yeah, we probably have bigger problems already with output
hot-unplugging, just an idea to keep in mind.

If you had a destroy signal in weston_output, could you not replace the
hash table lookups with a wl_signal_get()? Would that make anything
easier?


Thanks,
pq