Created attachment 323564 [details] [review]
wayland: Split out shell surface code from meta-wayland-surface.c

Move xdg_shell related functionality to a new meta-wayland-xdg-shell.c
and wl_shell related functionality to a new meta-wayland-wl-shell.c,
and adapt role object tree.

Common functionality related to the surface being drawn as a
MetaSurfaceActor was moved to a MetaWaylandSurfaceRoleActorSurface role.

The subsurface role GObject is made to inherit the actor surface GObject.

Shell surface hooks (configure, ping, close, popup done) were added to
a MetaWaylandSurfaceRoleShellSurface GObject which inherits the
surface actor role GObject.

The shell surface roles (xdg_surface, xdg_popup, wl_shell_surface) are
made to inherit the shell surface GObject and implement the relevant
API.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 323565 [details] [review]
wayland: Clean up wl_shell_surface popup management

The wl_surface_shell protocol allows changing the popup parent, so lets
deal with the possibility that it may happen.

Created attachment 323566 [details] [review]
wayland: Keep wl_shell_surface state during loss of window

It has been common practice (in QT5 for example) to set
wl_shell_surface state at situations where mutter will have destroyed
the MetaWindow. This commit keeps track of the relevant state
separately from MetaWindow, and synchronizes when needed.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 323567 [details] [review]
wayland: Let the roles handle their windows being managed

Move xdg_shell specific code from generic Wayland code into the xdg
shell code unit by letting the roles handle the corresponding
MetaWindow being managed.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 323568 [details] [review]
wayland: Set window type of wl_shell_surface popups to 'dropdown menu'

The wl_shell_surface popups are mostly used in the same way as
xdg_popup, so set the same window type.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 323569 [details] [review]
wayland: Remove wl_shell_surface parent destroy listener in destructor

If the surface is destroyed before the popup is dismissed, the destroy
listener needs to be removed from the parent destroy signal.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 323570 [details] [review]
wayland: Make xdg_shell surface role names shorter

MetaWaylandSurfaceRoleXdgSurface -> MetaWaylandXdgSurface
MetaWaylandSurfaceRoleXdgPopup -> MetaWaylandXdgPopup

Created attachment 323571 [details] [review]
wayland: Make wl_shell surface role name shorter

MetaWaylandSurfaceRoleWlShellSurface -> MetaWaylandWlShellSurface

Created attachment 323572 [details] [review]
wayland: Rename subsurface commit role function

This is to make it obvious it is an implementation of a role class
vfunc.

Created attachment 323573 [details] [review]
wayland: Add get_toplevel() vfunc to the role class

How to find the toplevel surface of a surface depends on the surface
role, so let the roles implement it themself.

Created attachment 323574 [details] [review]
wayland: Move out window state application into the roles

A large part of meta_wayland_surface_apply_window_state() was only
relevant for xdg_surface. Make this more obvious by splitting it up,
moving the relevant parts to the relevant roles.

Created attachment 323575 [details] [review]
wayland: Sync surface actor state in actor role commit handler

This'll also make the actor state already synchronized when shell
surfaces handlers apply their state.

Created attachment 323576 [details] [review]
MetaWindow: Make buffer_rect and rect share coordinate space

Before this commit, on Wayland, the buffer rect would have the size of
the attached Wayland buffer, no matter the scale. The scale would then
be applied ad-hoc by callers when a sane rectangle was needed. This
commit changes buffer_rect to rather represent the surface rect (i.e.
what is drawn on the stage, including client side shadow). The users of
buffer_rect will no longer need to scale the buffer_rect themself to
get a usable rectangle.

Created attachment 323577 [details] [review]
MetaSurfaceActorWayland: Only NULL check surface on class vfuncs

The only time the surface pointer (priv->surface) may be NULL is when
the surface is unmanaged but still painting, possibly due to a unmap
animation or the like, so only guard handle this situation in the entry
points that may come from the stage painting.

Created attachment 323578 [details] [review]
MetaSurfaceActorWayland: Use weak pointer instead of destroy hook

MetaWaylandSurface is a GObject now, so lets utilize that.

Created attachment 323579 [details] [review]
wayland: Simplify popup grabbing API

meta_wayland_popup_grab_create() creates and begins the grab and
meta_wayland_popup_grab_destroy() both ends and destroys the grab.

Created attachment 323580 [details] [review]
wayland/xdg-shell: Send popup_done if failed to start grab

Created attachment 323581 [details] [review]
wayland: Add 'MetaWaylandPopupSurface' bridge between popup and surface

Add a bridge between the MetaWaylandPopup object and the corresponding
popup surface role. This bridge replaces communicating dismissed and
unmapped popup events.

Created attachment 323582 [details] [review]
wayland: Let the popup surface explicitly dismiss the popup

Instead of relying on destroy signals attached to the corresponding
role object, let the roles explicitly dismiss the popup when it should
be dismissed.

Created attachment 323583 [details] [review]
wayland/wl_shell: Dismiss popup when parent is destroyed

Dismiss the popup when the parent is destroyed, and do this in the
destructor of the parent object. This makes the parent destory listener
unnecessary, since we already handle the parent child unlinking
explicitly in the object destructor.

Created attachment 323584 [details] [review]
wayland: Move shell surface role fields to the role structs

Don't keep all the role specific fields in MetaWaylandSurface and have
the roles manage the needed fields themself.

Created attachment 323585 [details] [review]
wayland/xdg-shell: Restructure file layout a bit

Separate "xdg_surface", "xdg_popup" and "xdg_shell" related functions
into three sections. Prior to this, the "xdg_shell" part was a bit all
over the place.

I finally did some testing (using https://github.com/jadahl/mutter/commits/wip/shell-surface-restructure). I ran a bunch of Qt5 demos, and everything worked great - in particular, menus, popups, etc. behaved as expected.

Thanks for the fixes!

*** Bug 757623 has been marked as a duplicate of this bug. ***

Review of attachment 323575 [details] [review]:

Looks good, but raises a question: shouldn't subsurface_role_commit() be chaining up to its parent class commit handler like other actor role subclasses ?

Review of attachment 323576 [details] [review]:

I'd prefer you renamed the variable if you want to change its meaning, it's called buffer_rect after all.

At the very least, the doc comment for meta_window_get_buffer_rect() needs to be updated to say surface instead of buffer for wayland windows.

Review of attachment 323577 [details] [review]:

ok

Review of attachment 323574 [details] [review]:

lgtm

Review of attachment 323573 [details] [review]:

fine

Review of attachment 323572 [details] [review]:

Hehe, this kinda adds to my argument on the previous patches to keep the role word around

Review of attachment 323571 [details] [review]:

Again, prefer role was kept, but ok

Review of attachment 323570 [details] [review]:

I'd prefer you kept the word role at least. But it's not a strong preference

Review of attachment 323568 [details] [review]:

ok

Review of attachment 323567 [details] [review]:

++

::: src/wayland/meta-wayland-surface.c
@@ +146,3 @@
 
+static void
+meta_wayland_surface_roll_shell_surface_managed (MetaWaylandSurfaceRoleShellSurface *shell_surface_role,

s/roll/role

@@ +1827,3 @@
 
+static void
+meta_wayland_surface_roll_shell_surface_managed (MetaWaylandSurfaceRoleShellSurface *shell_surface_role,

same

Review of attachment 323564 [details] [review]:

lgtm

::: src/wayland/meta-window-wayland.c
@@ +33,3 @@
 #include "meta-wayland-private.h"
 #include "meta-wayland-surface.h"
+#include "meta-wayland-xdg-shell.h"

unrelated

Review of attachment 323569 [details] [review]:

This should be squashed with

wayland: Clean up wl_shell_surface popup management

Review of attachment 323578 [details] [review]:

::: src/compositor/meta-surface-actor-wayland.c
@@ -480,3 @@
-    meta_surface_actor_wayland_get_instance_private (self);
-
-  wl_list_for_each_safe (callback, next, &priv->frame_callback_list, link)

where is ->frame_callback_list cleaned up now?

Review of attachment 323579 [details] [review]:

yes

Review of attachment 323581 [details] [review]:

nice

Review of attachment 323580 [details] [review]:

right

Review of attachment 323584 [details] [review]:

why isn't this squashed with the split in the first place?

Review of attachment 323585 [details] [review]:

could also be squashed

Review of attachment 323582 [details] [review]:

++

Review of attachment 323583 [details] [review]:

Doesn't this make

wayland: Clean up wl_shell_surface popup management

redundant? i.e. can't you drop that commit and fix the same bug its fixing with just this one?

Review of attachment 323565 [details] [review]:

looks ok but see the comment on

wayland/wl_shell: Dismiss popup when parent is destroyed

Review of attachment 323584 [details] [review]:

::: src/wayland/meta-wayland-surface.h
@@ -221,3 @@
-  /* wl_shell_surface */
-  struct {
-    MetaWlShellSurfaceState state;

the MetaWlShellSurfaceState enum should be moved from this file as well

Review of attachment 323566 [details] [review]:

::: src/wayland/meta-wayland-wl-shell.c
@@ +153,3 @@
 wl_shell_surface_set_state (MetaWaylandSurface     *surface,
+                            MetaWlShellSurfaceState state,
+                            gboolean                force)

this argument is never TRUE in any of the callers

@@ +227,3 @@
+                                             parent_surf->window,
+                                             x, y);
+    }

can't we re-use sync_wl_shell_parent_relationship() both here...

@@ +331,3 @@
+      meta_window_wayland_place_relative_to (surface->window,
+                                             parent_surf->window,
+                                             x, y);

... and here ? (re-use sync_wl_shell_parent_relationship())

(In reply to Rui Matos from comment #25)
> Review of attachment 323575 [details] [review] [review]:
> 
> Looks good, but raises a question: shouldn't subsurface_role_commit() be
> chaining up to its parent class commit handler like other actor role
> subclasses ?

Yes. It also makes frame callback managing in this function redundant. I will squash that into this patch.

(In reply to Rui Matos from comment #26)
> Review of attachment 323576 [details] [review] [review]:
> 
> I'd prefer you renamed the variable if you want to change its meaning, it's
> called buffer_rect after all.
> 
> At the very least, the doc comment for meta_window_get_buffer_rect() needs
> to be updated to say surface instead of buffer for wayland windows.

I didn't want to change any names because we kind of expose it (via meta_window_get_buffer_rect()) via our API. I'll add a comment about what it represents under the various display systems.

FWIW, we could come up with a better name (surface is not really correct either since that's not a thing on X11), and deprecate the accessor function, and the concept of "buffer_rect" completely from other places in the API. It at least doesn't seem to be used at all in gnome-shell (except that its passed around a bit before its ignored).

(In reply to Rui Matos from comment #37)
> Review of attachment 323578 [details] [review] [review]:
> 
> ::: src/compositor/meta-surface-actor-wayland.c
> @@ -480,3 @@
> -    meta_surface_actor_wayland_get_instance_private (self);
> -
> -  wl_list_for_each_safe (callback, next, &priv->frame_callback_list, link)
> 
> where is ->frame_callback_list cleaned up now?

It wasn't. I have a follow up patch that I'll squash into this one where its cleaned up the already there dispose function.

(In reply to Rui Matos from comment #41)
> Review of attachment 323584 [details] [review] [review]:
> 
> why isn't this squashed with the split in the first place?

Because the changes field move changed a lot of things that wasn't really related to moving between files. It'd be a gigantic change, where these changes wouldn't be easily spotted because they'd be part of a completely new file.

As for the popup changes, I suppose I can squash them together.

(In reply to Rui Matos from comment #47)
> Review of attachment 323566 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-wl-shell.c
> @@ +153,3 @@
>  wl_shell_surface_set_state (MetaWaylandSurface     *surface,
> +                            MetaWlShellSurfaceState state,
> +                            gboolean                force)
> 
> this argument is never TRUE in any of the callers
> 
> @@ +227,3 @@
> +                                             parent_surf->window,
> +                                             x, y);
> +    }
> 
> can't we re-use sync_wl_shell_parent_relationship() both here...
> 
> @@ +331,3 @@
> +      meta_window_wayland_place_relative_to (surface->window,
> +                                             parent_surf->window,
> +                                             x, y);
> 
> ... and here ? (re-use sync_wl_shell_parent_relationship())

Good point.

Created attachment 326910 [details] [review]
wayland: Clean up wl_shell_surface popup management

The wl_surface_shell protocol allows changing the popup parent, so lets
deal with the possibility that it may happen.

Created attachment 326911 [details] [review]
wayland: Keep wl_shell_surface state during loss of window

It has been common practice (in QT5 for example) to set
wl_shell_surface state at situations where mutter will have destroyed
the MetaWindow. This commit keeps track of the relevant state
separately from MetaWindow, and synchronizes when needed.

https://bugzilla.gnome.org/show_bug.cgi?id=757623

Created attachment 326912 [details] [review]
MetaWindow: Make buffer_rect and rect share coordinate space

Before this commit, on Wayland, the buffer rect would have the size of
the attached Wayland buffer, no matter the scale. The scale would then
be applied ad-hoc by callers when a sane rectangle was needed. This
commit changes buffer_rect to rather represent the surface rect (i.e.
what is drawn on the stage, including client side shadow). The users of
buffer_rect will no longer need to scale the buffer_rect themself to
get a usable rectangle.

Created attachment 326913 [details] [review]
MetaSurfaceActorWayland: Use weak pointer instead of destroy hook

MetaWaylandSurface is a GObject now, so lets utilize that.

I didn't squash 

wayland: Move shell surface role fields to the role structs
wayland/xdg-shell: Restructure file layout a bit

into any other commit because I think they serve a purpose being separate.

The newly attached ones have addressed issues you brought up.

The wl_shell_surface popup cleanup is still there, and that is mostly because the commit that continues that cleanup comes a lot later, and squashing them would be a lot of work doing history rewriting.

Review of attachment 326910 [details] [review]:

looks good

Review of attachment 326911 [details] [review]:

Just a small nit, feel free to ignore it.

::: src/wayland/meta-wayland-wl-shell.c
@@ +261,2 @@
 static void
+create_popup (MetaWaylandSurface *surface)

This function is only called from sync_wl_shell_parent_relationship() now so for clarity it should be moved down to be closer to where it's used.

Not a big deal though

Review of attachment 326912 [details] [review]:

ok, I think it's fine to keep buffer_rect as long as it's documented as it is now

::: src/core/window-private.h
@@ +419,3 @@
+   * For Wayland windows, the position matches the position of the
+   * surface associated with shell surface (wl_shell_surface, xdg_surface
+   * etc). The size matches the size surface size as displayed in the stage.

s/size surface size/surface size/

Review of attachment 326913 [details] [review]:

(In reply to Jonas Ådahl from comment #50)
> It wasn't. I have a follow up patch that I'll squash into this one where its
> cleaned up the already there dispose function.

If my eyes aren't tricking me, you attached the same patch

(In reply to Jonas Ådahl from comment #57)
> I didn't squash 
> 
> wayland: Move shell surface role fields to the role structs
> wayland/xdg-shell: Restructure file layout a bit
> 
> into any other commit because I think they serve a purpose being separate.
> 
> The newly attached ones have addressed issues you brought up.
> 
> The wl_shell_surface popup cleanup is still there, and that is mostly
> because the commit that continues that cleanup comes a lot later, and
> squashing them would be a lot of work doing history rewriting.

Fair enough, I suspected that the rebase conflicts were the reason.

Created attachment 327045 [details] [review]
MetaSurfaceActorWayland: Use weak pointer instead of destroy hook

MetaWaylandSurface is a GObject now, so lets utilize that.

(In reply to Rui Matos from comment #61)
> Review of attachment 326913 [details] [review] [review]:
> 
> (In reply to Jonas Ådahl from comment #50)
> > It wasn't. I have a follow up patch that I'll squash into this one where its
> > cleaned up the already there dispose function.
> 
> If my eyes aren't tricking me, you attached the same patch

No :P I had neatly placed the commit adding the framecallback cleanup in dispose after this one, but forgot to squash them together. Here is the squashed one.

Review of attachment 327045 [details] [review]:

++

Attachment 323564 [details] pushed as 7fd585f - wayland: Split out shell surface code from meta-wayland-surface.c
Attachment 323567 [details] pushed as f8878ac - wayland: Let the roles handle their windows being managed
Attachment 323568 [details] pushed as 1088bf4 - wayland: Set window type of wl_shell_surface popups to 'dropdown menu'
Attachment 323570 [details] pushed as 8755535 - wayland: Make xdg_shell surface role names shorter
Attachment 323571 [details] pushed as 23b1b5f - wayland: Make wl_shell surface role name shorter
Attachment 323572 [details] pushed as 5e54f32 - wayland: Rename subsurface commit role function
Attachment 323573 [details] pushed as e66f176 - wayland: Add get_toplevel() vfunc to the role class
Attachment 323574 [details] pushed as 9028e30 - wayland: Move out window state application into the roles
Attachment 323575 [details] pushed as ca44770 - wayland: Sync surface actor state in actor role commit handler
Attachment 323577 [details] pushed as 26815d6 - MetaSurfaceActorWayland: Only NULL check surface on class vfuncs
Attachment 323579 [details] pushed as e68b5f6 - wayland: Simplify popup grabbing API
Attachment 323580 [details] pushed as d9b98bc - wayland/xdg-shell: Send popup_done if failed to start grab
Attachment 323581 [details] pushed as 229a143 - wayland: Add 'MetaWaylandPopupSurface' bridge between popup and surface
Attachment 323582 [details] pushed as 61c717a - wayland: Let the popup surface explicitly dismiss the popup
Attachment 323583 [details] pushed as a89aa1d - wayland/wl_shell: Dismiss popup when parent is destroyed
Attachment 323584 [details] pushed as f318ec9 - wayland: Move shell surface role fields to the role structs
Attachment 323585 [details] pushed as a4ba72b - wayland/xdg-shell: Restructure file layout a bit
Attachment 326910 [details] pushed as b3ba8e8 - wayland: Clean up wl_shell_surface popup management
Attachment 326911 [details] pushed as c2643ba - wayland: Keep wl_shell_surface state during loss of window
Attachment 326912 [details] pushed as bca041b - MetaWindow: Make buffer_rect and rect share coordinate space
Attachment 327045 [details] pushed as 19f7e31 - MetaSurfaceActorWayland: Use weak pointer instead of destroy hook