Hi, I have minimize and maximize fully working in my next branch of gtk+ on github now. There are parts that depend on the new wayland core protocol shell_surface interface changes but these are so the clients can be controlled from the window list in the panel as well (i.e. toggle min/maximize from a right click menu in the panel). I figured I would submit it since it might still be useful. This branch works as-is using wayland/weston next repos in addition to gtk next.

https://github.com/soreau/gtk/commits/next

please add a patch here for review

Sure, here you go https://github.com/soreau/gtk/commit/2b95a45c8ae5bcc999679884f27ada71023d097f

This one might be good too. It would be good to consider cases such as LFS and gentoo that do use source directly from upstream. https://github.com/soreau/gtk/commit/27ffa17560f983dd12d9a17568e7100d2cdf829d It's also nice for users building from upstream to have the expected min/max/close buttons by default, without having to edit the theme straight away.

The protocol to actually hook up the maximize button is in place, and it would be relatively easy to hook up. See https://github.com/soreau/gtk/commit/8adf2b9ad9a79da1b1ad8b3732f86a4cc1dee101 for reference.

A link to github is not a patch... if you attach it here, we can use splinter to review it.

The changes for the events on the shell surface haven't yet hit wayland master. So i'd like to see just the maximising/iconifying as a separate patch.

Maybe I can make one when I have the time. Currently, I do not have the time unfortunately. Thanks for your interest.

commit 1be7f3dee93b0951204d147e1ab5f4cafe91b60f
Author: Scott Moreau <oreaus@gmail.com>
Date:   Sat Mar 16 17:34:57 2013 -0600

    wayland: Implement gdk_window_[un]maximize
    
    This allows the buttons in the decorations to maximise the window.
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=695945
    
    Signed-off-by: Rob Bradford <rob@linux.intel.com>

Keeping bug open:

- still need to implement minimize when it hits the protocol
- for maximise should preserve and save the size
- also need to make it react to the events when they appear

(In reply to comment #9)

Hi Rob, thanks for looking into this. I'm not sure why you put my name on this patch, it is not mine, it is yours. If you look, saved_width and saved_height are already in place for fullscreen and can be reused for maximize. I hooked all of this up in the github patch I posted here.

> Keeping bug open:
> 
> - still need to implement minimize when it hits the protocol

Naturally.

> - for maximise should preserve and save the size

This should have been already hooked up by this patch.

> - also need to make it react to the events when they appear

Not sure what you mean here.

(In reply to comment #8)
> commit 1be7f3dee93b0951204d147e1ab5f4cafe91b60f
> Author: Scott Moreau <oreaus@gmail.com>
> Date:   Sat Mar 16 17:34:57 2013 -0600
> 
>     wayland: Implement gdk_window_[un]maximize
> 
>     This allows the buttons in the decorations to maximise the window.
> 
>     Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=695945
> 
>     Signed-off-by: Rob Bradford <rob@linux.intel.com>

Just to be perfectly clear, I do not approve of this patch. It does not represent the intentions of my work.

Hi Scott, i'm sorry that the patch doesn't represent your intentions. In future when making changes to your patches i'll post them for review by yourself prior to including them in the tree. Your work is of course appreciated which is why I preserved your credit on the change.

On the point of using the same saved values for maximised as fullscreen I think a more complex solution is probably required.

We can picture some transitions like:

normal -> maximised -> fullscreen -> fullscreen -> maximised -> normal

or should we go fullscreen -> normal

We definitely need to preserve two sets of dimensions, but also probably need to consider that if we have been previously maximised and then fullscreened should we restore to maximised state (setting dimensions and calling wl_shell_surface_set_maximized) or should we go straight to the "normal" window mode.

This is conflated by the (hopefully soon) inclusion of the new events on shell_surface.

(In reply to comment #12)
> Hi Scott, i'm sorry that the patch doesn't represent your intentions. In future
> when making changes to your patches i'll post them for review by yourself prior
> to including them in the tree. Your work is of course appreciated which is why
> I preserved your credit on the change.

Yea, I was under the impression you'd actually make it work correctly but also use your name for the patch.. it's not too important really.

> 
> On the point of using the same saved values for maximised as fullscreen I think
> a more complex solution is probably required.
> 
> We can picture some transitions like:
> 
> normal -> maximised -> fullscreen -> fullscreen -> maximised -> normal
> 
> or should we go fullscreen -> normal

I just solved this for xwayland (testing with google-chrome). IMHO it should preserve the states as closely as the user put them. It should do normal -> maximised -> fullscreen -> fullscreen -> maximised -> normal, which is what X does but also xwayland too now that xwayland has maximize support. What I did for xwayland is:

1) saved_width/height are used for both fullscreen and maximize
2) I found a bug when doing normal -> maximized -> toggle CSD in google-chrome settings -> fullscreen -> unfullscreen -> unmaximize -> correct expected original size (normal). The problem was the saved_width/height are being reset on CSD toggle, so we're going to add a saved_size_valid var like weston's shell plugin has with saved_x/y and saved_position_valid. The variables are used for fullscreen and maximize.

> 
> We definitely need to preserve two sets of dimensions,

No, you do not. This is not the way it works with X and I'm not sure why you would think this. You can reuse the variables for both fullscreen and maximize. See https://github.com/soreau/weston/commit/d471441b7e7b8c71ca37398f354873bcd3a38a80 for possible reference and saved_x/y usage in shell plugin.

> but also probably need
> to consider that if we have been previously maximised and then fullscreened
> should we restore to maximised state (setting dimensions and calling
> wl_shell_surface_set_maximized) or should we go straight to the "normal" window
> mode.

You should never do nomral -> maximize -> fullscreen -> normal. This does not make a bit of sense and I'm not sure I know of an implementation that does this (google-chrome and any terminal, possibly any fullscree-capable app).

> 
> This is conflated by the (hopefully soon) inclusion of the new events on
> shell_surface.

Yes, wl_shell* wont last forever. I'm kinda surprised by your question here, it seems like a trick question. I expect someone of your position to know the answer to this one since it's plainly obvious by existing behavior and common sense. Your course of thought here is a bit puzzling.

- Scott

The plan for minimize/maximize has now changed to feature a proposal for an "xdg_shell" with a mechanism for the compositor to request to the client that it would like to be minimized.

(In reply to comment #14)
> The plan for minimize/maximize has now changed to feature a proposal for an
> "xdg_shell" with a mechanism for the compositor to request to the client that
> it would like to be minimized.

Thanks for the update. Is there any code or a written version of this proposal anywhere? I am wondering if this implementation would handle minimize differently than maximize.

Now that XWayland apps support minimizing in gnome-shell, it would be nice if native wayland apps did the same (the ones using gtk+3).

Created attachment 296287 [details] [review]
Implement Minimize in GDK Wayland backend

Review of attachment 296287 [details] [review]:

::: gdk/wayland/gdkwindow-wayland.c
@@ +1693,3 @@
+
+  if (GDK_WINDOW_IS_MAPPED (window))
+    xdg_surface_set_minimized (impl->xdg_surface);

We don't have the mapped check in any other window state setters in this file. If it is needed, that should be a separate bug-fix (for all the setters). But I don't think it is needed because the xdg_surface is created as part of mapping the window.

(In reply to Matthias Clasen from comment #18)
> Review of attachment 296287 [details] [review] [review]:
> 
> ::: gdk/wayland/gdkwindow-wayland.c
> @@ +1693,3 @@
> +
> +  if (GDK_WINDOW_IS_MAPPED (window))
> +    xdg_surface_set_minimized (impl->xdg_surface);
> 
> We don't have the mapped check in any other window state setters in this
> file. If it is needed, that should be a separate bug-fix (for all the
> setters). But I don't think it is needed because the xdg_surface is created
> as part of mapping the window.

I suppose I copy/pasted the check from the deiconify handler. It should be trivial to fix.

Created attachment 296550 [details] [review]
Implement Minimize in GDK Wayland backend