Thanks.
-----Original Message-----
From: Ander Conselvan de Oliveira [mailto:[email protected]]
Sent: Tuesday, December 03, 2013 11:06 PM
To: Zhang, Xiong Y
Cc: [email protected]
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 <[email protected]>
---
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
