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

[Giulio Camuffo]

2015-01-27 17:28 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
> 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))?

Oops, right.

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

I guess, but that would mean modifying the toplevel case, so i think
it should be a different patch. Making the maximized case
automatically configure the window wouldn't work out, the shell needs
to be involved.

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

window->shsurf is used without checking it in this same function
above, so i think we're pretty safe.

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

Thanks for the review,
Giulio