[PATCH 1/3] shell: restore app on non default and unplugged output
Zhang, Xiong Y
xiong.y.zhang at intel.com
Tue Dec 3 21:17:40 PST 2013
Hi, Ander:
Thanks for your review. Your idea is feasible.
>> To achieve that, we could add an output_destroyed() function pointer in weston_surface
But I think we could add an output_destroyed() function pointer in weston_view not in Weston_surface, because as Jason's idea one surface may have several views, what we moved is view not surface.
Because each output has a destroy_signal, we should register view's output_destroy_handler in Weston_view_assign_output(). So when one view is moved from one output to another output, the output_destroyed_handler should register again.
clamp_coordinate() is a common function and will be used in src/input.c and src/shell.c, I can implement a function weston_coordinate_clamp() in compositor.c and export it, is this OK ?
Thanks.
-----Original Message-----
From: Ander Conselvan de Oliveira [mailto:conselvan2 at gmail.com]
Sent: Tuesday, December 03, 2013 11:06 PM
To: Zhang, Xiong Y
Cc: wayland-devel at lists.freedesktop.org
Subject: Re: [PATCH 1/3] shell: restore app on non default and unplugged output
Hi Xiong,
It seems that we need different logic for moving the views depending on the surface role, so I'm thinking this code should actually be part of a surface role implementation. For instance, instead of implementing the logic of moving the pointer surface in the shell, that would go in src/input.c together with the implementation of pointer_cursor_surface_configure().
To achieve that, we could add an output_destroyed() function pointer in weston_surface, that would be called from an output_destroy_listener in weston_view. That should simplify the code below quite a bit since traversing the list of views would be done by the wl_signal_emit() code.
The output_destroyed() entry point could receive the new output as a parameter, so the role-specific logic would only have to determine where in that output the new surface should be and move it. And of course handle any role-specific needs, such as warping the pointer.
I have some comments about the coding style below.
On 12/03/2013 05:25 AM, Xiong Zhang wrote:
> if the unplugged output isn't the default output, move cursor and APP
> widow to default output
The commit message should be properly capitalized and punctuated.
>
> Signed-off-by: Xiong Zhang <xiong.y.zhang at intel.com>
> ---
> src/shell.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 186 insertions(+), 4 deletions(-)
>
> diff --git a/src/shell.c b/src/shell.c index dfcb525..16d22f1 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -6061,15 +6061,197 @@ workspace_move_surface_down_binding(struct weston_seat *seat, uint32_t time,
> take_surface_to_workspace_by_seat(shell, seat, new_index);
> }
>
> +/* if (*x, *y) will be moved beyong target_output, clamp it*/
Beyond is misspelled and there's a space missing in the end. I'd also recommend properly capitalizing and punctuating the sentences.
/* If (*x, *y) will be removed beyond target_output, clamp it. */
> +static int
> +clamp_coordinate(struct weston_output *target_output,
> + struct weston_output *original_output,
> + float *x, float *y)
The parameters should be aligned with the parenthesis, i.e.,
clamp_coordinate(struct weston_output *target_output,
struct weston_output *original_output,
> +{
> + int32_t rel_x, rel_y;
> + int result = 0;
> +
> + rel_x = *x - original_output->x;
> + rel_y = *y - original_output->y;
> +
> + if (rel_x > target_output->width) {
> + *x = original_output->x + target_output->width / 4;
> + result = 1;
> + }
> + if (rel_y > target_output->height) {
> + *y = original_output->y + target_output->height / 4;
> + result = 1;
> + }
> + return result;
> +}
> +
> +static void
> +handle_view_on_destroyed_output(struct weston_view *view,
> + struct weston_output *target_output) {
> + float new_x, new_y, original_x, original_y;
> +
> + /* the child window's view->geometry is a relative coordinate to */
> + /* parent view, no need to move child_view */
We format multi-line comments like this:
/* First line.
* Second line. */
> + if (view->geometry.parent)
> + return;
> +
> + original_x = view->geometry.x;
> + original_y = view->geometry.y;
> +
> + clamp_coordinate(target_output, view->output,
> + &original_x, &original_y);
> +
> + new_x = target_output->x + original_x - view->output->x;
> + new_y = target_output->y + original_y - view->output->y;
> + weston_view_set_position(view, new_x, new_y); }
> +
> +static void
> +normal_view_on_destroyed_output(struct shell_output *shell_output,
> + struct weston_output *target_output)
Parameter alignment.
> +{
> + struct weston_output *del_output = shell_output->output;
> + struct desktop_shell *shell = shell_output->shell;
> + struct weston_view *view;
> + unsigned int i;
> + struct workspace *ws;
> + struct shell_surface *shsurf;
> + enum shell_surface_type surface_type = SHELL_SURFACE_NONE;
> + struct wl_client *client;
> +
> + /*if view on destroyed output, move it to target output*/
Spaces around the comment.
> + for (i = 0; i < shell->workspaces.num; i++) {
> + ws = get_workspace(shell, i);
> + wl_list_for_each(view, &ws->layer.view_list, layer_link) {
> + if (view->output == del_output) {
> + handle_view_on_destroyed_output(view, target_output);
> + surface_type = get_shell_surface_type(view->surface);
A blank line here would make the whole block easier to read.
> + /*if surface is maximized, resize it to target output*/
> + if (surface_type == SHELL_SURFACE_MAXIMIZED) {
> + shsurf = get_shell_surface(view->surface);
> + shsurf->type = SHELL_SURFACE_NONE;
> + client = wl_resource_get_client(shsurf->resource);
> + shell_surface_set_maximized(client,
> + shsurf->resource,
> + wl_resource_find_for_client(&target_output->resource_list,
> +client));
We also align parameters for multi-line function calls with the parenthesis. This also breaks the 80 character limit. You can avoid this by having an extra variable for the client_resource, i.e.:
resource = wl_resource_find_for_client(); shell_surface_set_maximized(client, shsurf->resource,
resource);
> + }
> + }
> + }
> + }
> +}
> +
> +static void
> +full_screen_view_on_destroyed_output(struct shell_output *shell_output,
> + struct weston_output *target_output)
Parameter alignement.
> +{
> + struct weston_output *del_output = shell_output->output;
> + struct desktop_shell *shell = shell_output->shell;
> + struct weston_view *view;
> + struct shell_surface *shsurf;
> +
> + /*if view on destroyed output is fullscreen, resize it and associated black_view */
> + /*to target_output */
Comment format.
> + shsurf = NULL;
> + wl_list_for_each(view, &shell->fullscreen_layer.view_list, layer_link) {
> + /* fullscreen.black_view followed fullscreen.view */
Comment format.
> + if (shsurf && shsurf->fullscreen.black_view == view) {
> + handle_view_on_destroyed_output(view, target_output);
> + weston_view_configure(view, target_output->x, target_output->y,
> + target_output->width, target_output->height);
> + pixman_region32_fini(&view->surface->opaque);
> + pixman_region32_init_rect(&view->surface->opaque, 0, 0,
> + target_output->width, target_output->height);
> + pixman_region32_fini(&view->surface->input);
> + pixman_region32_init_rect(&view->surface->input, 0, 0,
> + target_output->width, target_output->height);
> + continue;
> + }
> + shsurf = get_shell_surface(view->surface);
> + if (shsurf->fullscreen_output == del_output) {
> + handle_view_on_destroyed_output(view, target_output);
> + shsurf->type = SHELL_SURFACE_NONE;
> + set_fullscreen(shsurf,
> + shsurf->fullscreen.type,
> + shsurf->fullscreen.framerate,
> + target_output);
> + }
> + }
> +}
> +
> +static void
> +pointer_on_destroyed_output(struct shell_output *shell_output,
> + struct weston_output *target_output)
Parameter alignment.
> +{
> + struct weston_output *del_output = shell_output->output;
> + struct weston_compositor *wc = del_output->compositor;
> + struct weston_seat *seat;
> + struct weston_pointer *pointer;
> + wl_fixed_t dx, dy;
> + float pointer_x, pointer_y;
> + struct wl_list *resource_list;
> + struct wl_resource *resource;
> +
> + /*if cursor exist in destroyed output, issue a fake motion to move
> +cursor to target_output*/
Comment format.
> + wl_list_for_each(seat, &wc->seat_list, link) {
> + if (!seat->pointer)
> + continue;
This whole block could use some blank lines to make it easier to read.
For instance, one here, and also ...
> + pointer = seat->pointer;
> + pointer_x = wl_fixed_to_double(pointer->x);
> + pointer_y = wl_fixed_to_double(pointer->y);
here,
> + if (!pixman_region32_contains_point(&del_output->region,
> + pointer_x, pointer_y, NULL))
> + continue;
here,
> + if (clamp_coordinate(target_output, del_output, &pointer_x, &pointer_y)) {
> + pointer->x = wl_fixed_from_double(pointer_x);
> + pointer->y = wl_fixed_from_double(pointer_y);
> + }
here,
> + dx = wl_fixed_from_int(target_output->x - del_output->x);
> + dy = wl_fixed_from_int(target_output->y - del_output->y);
> + pointer->x += dx;
> + pointer->y += dy;
here,
> + if (pointer->sprite)
> + handle_view_on_destroyed_output(pointer->sprite, target_output);
here
> + pointer->grab->interface->focus(pointer->grab);
> + resource_list = &pointer->focus_resource_list;
and here.
> + wl_resource_for_each(resource, resource_list) {
> + weston_view_from_global_fixed(pointer->focus,
> + pointer->x, pointer->y,
> + &dx, &dy);
> + wl_pointer_send_motion(resource,
> + weston_compositor_get_time(),
> + dx, dy);
> + }
> + }
> +}
> +
> static void
> handle_output_destroy(struct wl_listener *listener, void *data)
> {
> - struct shell_output *output_listener =
> + struct shell_output *shell_output =
> container_of(listener, struct shell_output, destroy_listener);
> + struct weston_output *output = (struct weston_output *)data;
> + struct weston_compositor *wc = output->compositor;
> + struct weston_output *default_output, *target_output = NULL;
> +
> + default_output = get_default_output(wc);
> + /* if default output is the only output in output_list, or destroyed
> + output is default output, return*/
Comment format.
> + if ((default_output->link.prev == default_output->link.next) ||
> + (default_output == output))
Align "(default_output) ..." with the parenthesis. Also, the goto below is really unnecessary. You could just revert the condition and put the code until the exit_handler label inside the if.
> + goto exit_handler;
> +
> + target_output = default_output;
> +
> + normal_view_on_destroyed_output(shell_output, target_output);
> + full_screen_view_on_destroyed_output(shell_output, target_output);
> + pointer_on_destroyed_output(shell_output, target_output);
> +
> + weston_output_schedule_repaint(target_output);
>
> - wl_list_remove(&output_listener->destroy_listener.link);
> - wl_list_remove(&output_listener->link);
> - free(output_listener);
> +exit_handler:
> + wl_list_remove(&shell_output->destroy_listener.link);
> + wl_list_remove(&shell_output->link);
> + free(shell_output);
> }
>
> static void
>
More information about the wayland-devel
mailing list