[PATCH weston v2 2/2] xwm: support maximizing xwayland windows

[Daniel Stone]

Hi,

On 9 January 2015 at 18:10, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> +               if (maximized != window->maximized_vert && window->maximized_horz) {

Surely this needs to be:
if (maximized != (window->maximized_vert && window->maximized_horz))?

You could break this out into a separate local variable to minimise
(ha) the repetition.

> +                       if (window->maximized_vert && window->maximized_horz) {
> +                               window->saved_width = window->width;
> +                               window->saved_height = window->height;
> +
> +                               if (window->shsurf)
> +                                       shell_interface->set_maximized(window->shsurf);
> +                       } else {
> +                               weston_wm_window_set_toplevel(window);

This seems to drop the if (window->shsurf) test from previous.

> @@ -1667,6 +1709,19 @@ weston_wm_handle_button(struct weston_wm *wm, xcb_generic_event_t *event)
>                 weston_wm_window_close(window, button->time);
>                 frame_status_clear(window->frame, FRAME_STATUS_CLOSE);
>         }
> +
> +       if (frame_status(window->frame) & FRAME_STATUS_MAXIMIZE) {
> +               window->maximized_horz = !window->maximized_horz;
> +               window->maximized_vert = !window->maximized_vert;
> +               if (window->maximized_horz && window->maximized_vert) {
> +                       window->saved_width = window->width;
> +                       window->saved_height = window->height;
> +                       shell_interface->set_maximized(window->shsurf);

Do we need to call weston_wm_window_configure here?

... oh, no, we don't, as the shell_surface->set_maximized() hook
already does that for us. The lack of symmetry with ->set_toplevel()
there is a bit confusing; it'd be good to make them consistent.

> +               } else {
> +                       weston_wm_window_set_toplevel(window);
> +               }

This duplicates code from above, and again, do we need to test for
window->shsurf here, or are we already guaranteed it?

Other than that, this looks good to me; with those fixed (or explained), then:
Reviewed-by: Daniel Stone <daniels at collabora.com>

I'm emphatically not a WM person, mind ...

Cheers,
Daniel