[PATCH 1/3] shell: restore app on non default and unplugged output
Ander Conselvan de Oliveira
conselvan2 at gmail.com
Tue Dec 3 23:42:25 PST 2013
On 12/04/2013 07:17 AM, Zhang, Xiong Y wrote:
> 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.
Right, it would be more natural to have the function pointer in the
view. However, then we need to make sure that whenever a view for that
surface is created, it has the proper output_destroyed() pointer set.
Since this is related to the surface role, I think it would be good if
it was implemented in surface next to the configure function pointer,
but I think either way is fine.
> 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 ?
Yep sure. We actually have a similar function that is exported,
weston_pointer_clamp(). It doesn't do what you need, since it takes a
weston_pointer as argument, but you could split the actual clamping into
the function you need and export it.
Cheers,
Ander
>
> 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