(I'm the maintainer of wayland-window, the project on which this bug occured.)

It appears the issue is that the pointer grab associated with the serial was from a subsurface, while the resize/move request are sent on the parent surface. (the wayland-window crate uses subsurfaces to draw its decorations).

Weston allows this, but apparently mutter refuses it, because the grab and the request are not associated with the same surface, if I'm correctly understanding the meta_wayland_pointer_can_grab_surface function.

It seems that the correct fix for this would be to have `meta_wayland_pointer_can_grab_surface` return `true` if either `pointer->focus_surface == surface` **or** if `pointer->focus_surface` is equal to any of the `surface`'s subsurfaces. Could any of the mutter devs comment on whether this sounds correct or not?

(In reply to mindtree from comment #2)
> It seems that the correct fix for this would be to have
> `meta_wayland_pointer_can_grab_surface` return `true` if either
> `pointer->focus_surface == surface` **or** if `pointer->focus_surface` is
> equal to any of the `surface`'s subsurfaces. Could any of the mutter devs
> comment on whether this sounds correct or not?

This sounds reasonable. Same would apply for touch too.

mindtree, are you working on a fix for this?

Jonas Ådahl, I had planned to however it looks like I won't have time at least for a couple weeks. I'm also not familiar with the gnome/mutter build system so it would likely take me a lot longer than somebody already familiar with mutter.

If you (or anyone else) would like to go ahead and make the fix that would be greatly appreciated!

I'd also be more than happy to help test the fix if necessary.

Has there been any progress on this issue? If not, does anyone have plans to address it?

(In reply to mindtree from comment #6)
> Has there been any progress on this issue? If not, does anyone have plans to
> address it?

I don't know about others, but I haven't to it yet. If you want to work on this, please feel free to do so.

Hey Jonas, I had a go at getting this working but couldn't quite get a local build of mutter to run correctly. This is the final error that I ran into before running out of time:

```
(mutter:11472): Gdk-CRITICAL **: gdk_screen_get_setting: assertion 'GDK_IS_SCREEN (screen)' failed

(mutter:11472): mutter-ERROR **: Failed to spawn Xwayland: Failed to execute child process “/home/mindtree/jhbuild/install/bin/Xwayland” (No such file or directory)
Trace/breakpoint trap (core dumped)
```

The command used to try and run mutter is as follows

```
jhbuild run mutter --nested --wayland
```

I rebuilt mutter as you suggested on IRC after running `./configure` and passing the xwayland path argument as you suggested and it seemed to complete successfully, however the error above continued to occur when I tried running it again.

It seems like I might not have time to get back to this for a while so please feel free to take this on. Would be much appreciated!

(In reply to mindtree from comment #8)

> (mutter:11472): mutter-ERROR **: Failed to spawn Xwayland: Failed to execute
> child process “/home/mindtree/jhbuild/install/bin/Xwayland” (No such file or
> directory)
> Trace/breakpoint trap (core dumped)

This means you don't have Xwayland in your jhbuild prefix. Something like

  module_autogenargs['mutter']='--with-xwayland-path=/usr/bin/Xwayland'

in ~/.config/jhbuildrc should get you past that error (after rebuilding mutter of course).

(In reply to Florian Müllner from comment #9)
> (In reply to mindtree from comment #8)
> 
> > (mutter:11472): mutter-ERROR **: Failed to spawn Xwayland: Failed to execute
> > child process “/home/mindtree/jhbuild/install/bin/Xwayland” (No such file or
> > directory)
> > Trace/breakpoint trap (core dumped)
> 
> This means you don't have Xwayland in your jhbuild prefix. Something like
> 
>   module_autogenargs['mutter']='--with-xwayland-path=/usr/bin/Xwayland'
> 
> in ~/.config/jhbuildrc should get you past that error (after rebuilding
> mutter of course).

Thanks for your suggestion, it worked! I think I've successfully managed to build and run an instance of mutter using jhbuild.

I've began implementing this, however I have some questions with regards to how `meta_wayland_pointer_can_grab_surface` should behave:

1. Can a subsurface of a `MetaWaylandSurface` also have its own subsurfaces?
2. If so, when checking subsurfaces, should only the immediate subsurfaces be checked? Or should the entire tree of subsurfaces (including subsurfaces of subsurfaces) be checked in some sort of traversal such as DFS or BFS?
3. If so, how should this be performed? Are there tree traversal utilities included within the repository somewhere? Should the traversal use the call stack via recursion? Or is there a gnome stack type that should be used?

Any advice on this appreciated.

As a side-note, is there somewhere suitable that I may offer a bounty for solving this issue in the case that I cannot manage to myself?

I decided to implement this conservatively for now so that only the immediate subsurfaces are checked (along with a note above the code explaining this).

It works!!! I can now move and resize the window successfully after this patch :)

Here is the diff for the patch:


-  return (pointer->grab_serial == serial &&
-          pointer->focus_surface == surface);
+  if (pointer->grab_serial == serial)
+    {
+      if (pointer->focus_surface == surface)
+        return true;
+
+      // Check if focus matches any of the subsurfaces.
+      // NOTE: This only checks the immediate subsurfaces.
+      // Should we be checking the whole tree using some sort of traversal?
+      for (GList *l = surface->subsurfaces; l != NULL; l = l->next)
+        {
+          MetaWaylandSurface *subsurface = l->data;
+          if (pointer->focus_surface == subsurface)
+            return true;
+        }
+    }
+
+  return false;


Does this look OK? If so, how may I go about submitting a pull request? Any advice appreciated!

For the record, the wayland documentation of the wl_subcompositor:

> The root surface in a tree of sub-surfaces is the main
> surface. The main surface cannot be a sub-surface, because
> sub-surfaces must always have a parent.
>
> A main surface with its sub-surfaces forms a (compound) window.
> For window management purposes, this set of wl_surface objects is
> to be considered as a single window, and it should also behave as
> such.

My understanding here is that the whole tree of subsurfaces is tha "compound window", and as such the logic would require traversal of the entire subsurface tree.

(In reply to mindtree from comment #11)
> I decided to implement this conservatively for now so that only the
> immediate subsurfaces are checked (along with a note above the code
> explaining this).
> 
> It works!!! I can now move and resize the window successfully after this
> patch :)
> 
> Here is the diff for the patch:
> 
> 
> -  return (pointer->grab_serial == serial &&
> -          pointer->focus_surface == surface);
> +  if (pointer->grab_serial == serial)
> +    {
> +      if (pointer->focus_surface == surface)
> +        return true;
> +
> +      // Check if focus matches any of the subsurfaces.
> +      // NOTE: This only checks the immediate subsurfaces.
> +      // Should we be checking the whole tree using some sort of traversal?
> +      for (GList *l = surface->subsurfaces; l != NULL; l = l->next)
> +        {
> +          MetaWaylandSurface *subsurface = l->data;
> +          if (pointer->focus_surface == subsurface)
> +            return true;
> +        }
> +    }
> +
> +  return false;
> 
> 
> Does this look OK? If so, how may I go about submitting a pull request? Any
> advice appreciated!

In general yes, though I suggest calling can_grab() recursively on all subsurfaces. That'll also make the code self explanatory, thus no need for any comments. In short, it could for example be something similar to
"can_grab(..)
{
  if (pointer->grab_serial != serial)
    return FALSE;

 if (pointer->focus_surface == surface)
   return TRUE;

  forall (subsurfaces)
    {
      if (can_grab(subsurface))
        return TRUE;
    }

  return FALSE;
}"

When you have a commit on your local git clone, you can use "git-bz" to upload it as a patch to this bug. Then I can do a real code review. We don't support pull requests anywhere yet.

If you want, you can implement the equivalent thing for touch, but I guess that'd require you to have a touch device.

Thanks for working on this!

(In reply to Levans from comment #12)
> For the record, the wayland documentation of the wl_subcompositor:
> 
> > The root surface in a tree of sub-surfaces is the main
> > surface. The main surface cannot be a sub-surface, because
> > sub-surfaces must always have a parent.
> >
> > A main surface with its sub-surfaces forms a (compound) window.
> > For window management purposes, this set of wl_surface objects is
> > to be considered as a single window, and it should also behave as
> > such.
> 
> My understanding here is that the whole tree of subsurfaces is tha "compound
> window", and as such the logic would require traversal of the entire
> subsurface tree.

Indeed.

Created attachment 353046 [details] [review]
Fix meta_wayland_pointer_can_grab_surface - Bug #781811.

Previously, the function only returned `true` if the given surface was
equal to the given pointer's focused surface. This changes the behaviour
to also return `true` if any of the given surface's subsurfaces are
equal to the pointer's focused surface.

Review of attachment 353046 [details] [review]:

Looks good, I just have coding style nits.

The commit message needs fixing though. To be conformant, it should be something like:

wayland/pointer: Handle subsurfaces when grabbing
 
Previously, the function only returned `true` if the given surface was
equal to the given pointer's focused surface. This changes the behaviour
to also return `true` if any of the given surface's subsurfaces are
equal to the pointer's focused surface.

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


But feel free to come up with a better subject.

::: src/wayland/meta-wayland-pointer.c
@@ +1124,3 @@
+{
+  if (pointer->focus_surface == surface)
+    return true;

TRUE

@@ +1126,3 @@
+    return true;
+
+  for (GList *l = surface->subsurfaces; l != NULL; l = l->next)

Variable declarations on top. "l != NULL" is usually written as just "l" in these types of loops but doesn't matter.

@@ +1128,3 @@
+  for (GList *l = surface->subsurfaces; l != NULL; l = l->next)
+    {
+      MetaWaylandSurface *subsurface = l->data;

Empty line after declarations.

@@ +1129,3 @@
+    {
+      MetaWaylandSurface *subsurface = l->data;
+      if (pointer_can_grab_surface(pointer, subsurface))

Space between function name and (

@@ +1130,3 @@
+      MetaWaylandSurface *subsurface = l->data;
+      if (pointer_can_grab_surface(pointer, subsurface))
+        return true;

TRUE

@@ +1133,3 @@
+    }
+
+  return false;

FALSE

Created attachment 353047 [details] [review]
wayland/pointer: Check for subsurfaces when grabbing

Previously, the function only returned `TRUE` if the given surface was
equal to the given pointer's focused surface. This changes the behaviour
to also return `TRUE` if any of the given surface's subsurfaces are
equal to the pointer's focused surface.

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

(In reply to Jonas Ådahl from comment #15)
> Review of attachment 353046 [details] [review] [review]:
> 
> Looks good, I just have coding style nits.
> 
> The commit message needs fixing though. To be conformant, it should be
> something like:
> 
> wayland/pointer: Handle subsurfaces when grabbing
>  
> Previously, the function only returned `true` if the given surface was
> equal to the given pointer's focused surface. This changes the behaviour
> to also return `true` if any of the given surface's subsurfaces are
> equal to the pointer's focused surface.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=781811.
> 
> 
> But feel free to come up with a better subject.
> 
> ::: src/wayland/meta-wayland-pointer.c
> @@ +1124,3 @@
> +{
> +  if (pointer->focus_surface == surface)
> +    return true;
> 
> TRUE
> 
> @@ +1126,3 @@
> +    return true;
> +
> +  for (GList *l = surface->subsurfaces; l != NULL; l = l->next)
> 
> Variable declarations on top. "l != NULL" is usually written as just "l" in
> these types of loops but doesn't matter.
> 
> @@ +1128,3 @@
> +  for (GList *l = surface->subsurfaces; l != NULL; l = l->next)
> +    {
> +      MetaWaylandSurface *subsurface = l->data;
> 
> Empty line after declarations.
> 
> @@ +1129,3 @@
> +    {
> +      MetaWaylandSurface *subsurface = l->data;
> +      if (pointer_can_grab_surface(pointer, subsurface))
> 
> Space between function name and (
> 
> @@ +1130,3 @@
> +      MetaWaylandSurface *subsurface = l->data;
> +      if (pointer_can_grab_surface(pointer, subsurface))
> +        return true;
> 
> TRUE
> 
> @@ +1133,3 @@
> +    }
> +
> +  return false;
> 
> FALSE

Just wanted to say thanks for the feedback - it has been a long time since I've written C!

I have attached a new patch including all of your suggested amendments.

Review of attachment 353047 [details] [review]:

looks goot to me, thanks!

No problem!

Could you possibly give me an indication of how long I can expect to wait for this to land in stable? I'm particularly curious about how long these sorts of changes take to land in the arch repos. Thanks!

I'll go a head and land it in the 3.24 branch and master, so it'll be part of the next release.

Pushed to to gnome-3-24 and master.

I noticed you had put a "." after the bugzilla link. That caused git-bz to fail
to understand what bug the patch was associated with. So next time, just the
link on its own line, without any punctuation, or just use git-bz to do it all
automatically. I fixed the link on the gnome-3-24 branch, but on master I
discovered it too late, so it'll still have the ".".

Attachment 353047 [details] pushed as e22c753 - wayland/pointer: Check for subsurfaces when grabbing