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

[Jonas Ådahl]

On Mon, Jan 28, 2013 at 10:33 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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.

Sounds like a good precaution, until we handle the case properly.

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

I think it makes sense to dynamically remove/add workspace containers
when outputs are disconnected/connected. Surfaces in an container that
was associated with an output would need to be re-stacked and
repositioned somewhere else.

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

The hash table would still be used for accessing container when
handling the protocol requests. I also think it would make sense to
not practically limit the workspace container map to outputs, for
example the case when we have no outputs, we probably still want to
have some off-screen container.

>
>
> Thanks,
> pq

Thanks for the input,

Jonas