Created attachment 299277 [details] [review]
Rename MetaCursorReference to MetaCursorSprite

It isn't really a reference to a MetaCursor enum, but a reference
counted cursor sprite, and enum value.

Created attachment 299278 [details] [review]
Make MetaCursorSprite a GObject

In order to abstract away some parts, make the cursor sprite object a
proper GObject, so that we can create backend specific types with the
same API.

Created attachment 299279 [details] [review]
backends: Move some function declarations from meta-cursor-private.h

Don't include meta-cursor-private.h from anywhere except the cursor
implementation file. The places where it was included only used two
function definitions in the file, so move those to meta-cursor.h.

Created attachment 299280 [details] [review]
MetaCursorSprite: Put renderer specific code in the renderer

There were lots of code handling the native renderer specific cases;
move these parts to the renderer. Note that this causes the X11 case to
always generate the texture which is a waste of memory, but his
regression will be fixed in a following commit.

Created attachment 299281 [details] [review]
Move meta-cursor-* files to new frontends/ directory

The idea is to put frontend interfaces in src/frontends/ and their
implementations src/frontends/x11/ or src/frontends/wayland/. While a
backend is how mutter draws to the screen, the frontend is how mutter
communicates with its clients.

Created attachment 299282 [details] [review]
MetaCursorSprite: Move Wayland part to own its frontend implementation

Created attachment 299283 [details] [review]
MetaMonitorInfo: Provide scale information

Created attachment 299284 [details] [review]
MetaMonitorManager: Fix comment

Created attachment 299285 [details] [review]
wayland: Paint proper pointer cursor size on HiDPI monitors

When a poiner is on a HiDPI monitor, scale the pointer sprite according
to the monitor scale. This currently assumes that the system is
configured to specify the theme size in standard DPI (meaning the theme
should assume the output scale is 1).

This works by making MetaCursorSprite more state aware during painting so
that it, given what monitor it is on and the output scale of that
monitor, can scale surface based surfaces accordingly, or recreate
server side cursor images by loading a larger version.

The native cursor renderer is changed to store up to two gbm_bo's per
MetaCursorSprite in order to draw a larger version while showing the
previous one.

Created attachment 299286 [details] [review]
MetaCursorSprite: Move out X11 part to its own frontend implementation

Created attachment 299287 [details] [review]
MetaCursorSprite: Move X11 only constructor to X11 implementation

meta_cursor_sprite_from_texture was only used when running as a X11 CM.

Created attachment 299288 [details] [review]
wayland: Send wl_surface.enter/leave to cursor surfaces

In order for clients to be able to attach high resolution pointer cursor
buffers, they need to know what buffer scales they should use.

Related GTK+ patches: https://bugzilla.gnome.org/show_bug.cgi?id=746141

The same comment about global HiDPI configuration applies to these patches.

this needs to go to mutter, I think

Review of attachment 299288 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +93,3 @@
 }
 
+

Remove the double whitespace.

::: src/wayland/meta-wayland-surface.c
@@ +891,3 @@
+    {
+    case META_WAYLAND_SURFACE_ROLE_NONE:
+

Remove the funky whitespace here.

@@ +910,1 @@
   set_surface_is_on_output (surface, wayland_output, is_on_output);

Missing a newline here.

Comment on attachment 299284 [details] [review]
MetaMonitorManager: Fix comment

Attachment 299284 [details] pushed as aac5a5d - MetaMonitorManager: Fix comment

Created attachment 300334 [details] [review]
wayland: Send wl_surface.enter/leave to cursor surfaces

In order for clients to be able to attach high resolution pointer cursor
buffers, they need to know what buffer scales they should use.

Created attachment 301060 [details] [review]
wayland: Paint proper pointer cursor size on HiDPI monitors

When a poiner is on a HiDPI monitor, scale the pointer sprite according
to the monitor scale. This currently assumes that the system is
configured to specify the theme size in standard DPI (meaning the theme
should assume the output scale is 1).

This works by making MetaCursorSprite more state aware during painting so
that it, given what monitor it is on and the output scale of that
monitor, can scale surface based surfaces accordingly, or recreate
server side cursor images by loading a larger version.

The native cursor renderer is changed to store up to two gbm_bo's per
MetaCursorSprite in order to draw a larger version while showing the
previous one.


-------

This new version removes the surface_destory_listener in the
MetaCursorSpriteWayland object. It was not needed as it will
always be destoryed before the surface is, since we always update
the cursor before the surface is destroyed.

Review of attachment 299277 [details] [review]:

Yeah, this is a better name for it.

Review of attachment 299278 [details] [review]:

Minor fixes, otherwise fine.

::: src/backends/meta-cursor-tracker.c
@@ +86,3 @@
     return;
 
+  g_clear_pointer (&tracker->displayed_cursor, g_object_unref);

Use g_clear_object.

@@ +164,3 @@
                    MetaCursorSprite  *cursor)
 {
+  g_clear_pointer (&tracker->window_cursor, g_object_unref);

Same.

@@ +191,3 @@
     return FALSE;
 
+  g_clear_pointer (&tracker->xfixes_cursor, g_object_unref);

Same.

@@ +345,3 @@
                                      MetaCursorSprite  *cursor)
 {
+  g_clear_pointer (&tracker->root_cursor, g_object_unref);

Same.

Review of attachment 299278 [details] [review]:

::: src/backends/meta-cursor-tracker.c
@@ +90,3 @@
+    {
+      g_object_ref (displayed_cursor);
+      tracker->displayed_cursor = displayed_cursor;

g_object_returns the ref'ed object as well - not sure if this change is because you aren't aware of that or object to that stylistically.

::: src/backends/meta-cursor.c
@@ +54,3 @@
+} MetaCursorImage;
+
+struct _MetaCursorSpritePrivate

This is OK, but to express my opinion, I don't really see any reason to use a private structure - private structures are basically useful for publicly derivable objects where you need to have the object class in a public header. MetaCursorSprite isn't derivable and it isn't even public.

Review of attachment 299279 [details] [review]:

Sure

(In reply to Owen Taylor from comment #21)
> Review of attachment 299278 [details] [review] [review]:
> 
> ::: src/backends/meta-cursor-tracker.c
> @@ +90,3 @@
> +    {
> +      g_object_ref (displayed_cursor);
> +      tracker->displayed_cursor = displayed_cursor;
> 
> g_object_returns the ref'ed object as well - not sure if this change is
> because you aren't aware of that or object to that stylistically.

Is it a stylistic guideline to do o = g_object_ref (o)? I was aware of it returning the object, but assumed relying on the side effect was enough. If the guideline is to do o = g_object_ref (o) I can change it.

> 
> ::: src/backends/meta-cursor.c
> @@ +54,3 @@
> +} MetaCursorImage;
> +
> +struct _MetaCursorSpritePrivate
> 
> This is OK, but to express my opinion, I don't really see any reason to use
> a private structure - private structures are basically useful for publicly
> derivable objects where you need to have the object class in a public
> header. MetaCursorSprite isn't derivable and it isn't even public.

The point of making it private is to not tempt anyone to use cursor_sprite->internal_data and instead use a provided API. I think this makes sense even if the object is not exposed publicly.

(In reply to Jonas Ådahl from comment #23)
> (In reply to Owen Taylor from comment #21)
> > Review of attachment 299278 [details] [review] [review] [review]:
> > 
> > ::: src/backends/meta-cursor-tracker.c
> > @@ +90,3 @@
> > +    {
> > +      g_object_ref (displayed_cursor);
> > +      tracker->displayed_cursor = displayed_cursor;
> > 
> > g_object_returns the ref'ed object as well - not sure if this change is
> > because you aren't aware of that or object to that stylistically.
> 
> Is it a stylistic guideline to do o = g_object_ref (o)? I was aware of it
> returning the object, but assumed relying on the side effect was enough. If
> the guideline is to do o = g_object_ref (o) I can change it.

You'd find that the vast majority of uses of g_object_ref() in mutter do use the return value - it's very common throughout GNOME and certainly how I'd write this type of code. I probably wouldn't have commented it if you just did it in two lines in new code - the comment was inspired by the fact that you changed code using a return value (for a custom ref function) to not using the return value and that made me do a double take.... it seemed like a bigger delta than necessary.

In addition to just saving a line of code, the reason I like using the return value is that it makes it clear that assigning a newly acquired reference is a single conceptual action - the assignment can't become accidentally detached from the reference.

> > ::: src/backends/meta-cursor.c
> > @@ +54,3 @@
> > +} MetaCursorImage;
> > +
> > +struct _MetaCursorSpritePrivate
> > 
> > This is OK, but to express my opinion, I don't really see any reason to use
> > a private structure - private structures are basically useful for publicly
> > derivable objects where you need to have the object class in a public
> > header. MetaCursorSprite isn't derivable and it isn't even public.
> 
> The point of making it private is to not tempt anyone to use
> cursor_sprite->internal_data and instead use a provided API. I think this
> makes sense even if the object is not exposed publicly.

The same could be done by moving the MetaCursor structure meta-cursor.c. I don't have a real objection to using a private data structure - there is certainly no significant performance penalty. I think a lot of people like to just use them everywhere to avoid having different patterns of writing objects depending on whether they are publicly derivable or not.

(In reply to Owen Taylor from comment #24)
> (In reply to Jonas Ådahl from comment #23)
> > 
> > Is it a stylistic guideline to do o = g_object_ref (o)? I was aware of it
> > returning the object, but assumed relying on the side effect was enough. If
> > the guideline is to do o = g_object_ref (o) I can change it.
> 
> You'd find that the vast majority of uses of g_object_ref() in mutter do use
> the return value - it's very common throughout GNOME and certainly how I'd
> write this type of code. I probably wouldn't have commented it if you just
> did it in two lines in new code - the comment was inspired by the fact that
> you changed code using a return value (for a custom ref function) to not
> using the return value and that made me do a double take.... it seemed like
> a bigger delta than necessary.
> 
> In addition to just saving a line of code, the reason I like using the
> return value is that it makes it clear that assigning a newly acquired
> reference is a single conceptual action - the assignment can't become
> accidentally detached from the reference.

Will change it and remember the preferred usage of g_object_ref in the future.

> > 
> > The point of making it private is to not tempt anyone to use
> > cursor_sprite->internal_data and instead use a provided API. I think this
> > makes sense even if the object is not exposed publicly.
> 
> The same could be done by moving the MetaCursor structure meta-cursor.c. I
> don't have a real objection to using a private data structure - there is
> certainly no significant performance penalty. I think a lot of people like
> to just use them everywhere to avoid having different patterns of writing
> objects depending on whether they are publicly derivable or not.

I can't move the MetaCursorSprite structure to meta-cursor.c because I inherit it in MetaCursorWayland in a later patch.

Just in case it helps testing: the adwaita cursor theme in master now has 'hi-dpi' sizes: 64 (=32x2) and 96 (=48x2).

Review of attachment 299283 [details] [review]:

See comments for same patch on bug 744934

So this is a pretty big patch set - so I wanted to stop back and discuss where we want to go with this for 3.16.

 * Is there some simpler patch that just fixes the initial bug report and gets us to the point where we aren't regressing from X and using the wrong cursor size when there's only one monitor? It looks like GTK+ tries to create a cursor of the right size. What if we just used the cursor set by the application pixel-for-pixel ignoring the scale set on it? (Or are we already doing that and the problem is the lack of proper enter/leave events?)

 * If we want to land this full set of patches, what I think we should do is wait until 3.16.1 (which is Monday, I think anyways?) and mutter is branched, and land them on master, and then look at backporting once they've had a little chance to get some testing and mature.

Review of attachment 299281 [details] [review]:

Hmm, in the context of trying to get 3.16.x working well in a hidpi environment, this seems like it's a bit of an extraneous refactor. 

I dislike the frontends/ naming - I think it's "clever" rather than informative rather than informative. There's a pretty well established idea that the "frontend" of a system is on the user-facing side (http://en.wikipedia.org/wiki/Front_and_back_ends), so I think people are not going to look in a frontends/ directory for the code relating to protocols. I think this would be better called protocols/.

From IRC <Jasper> And protocol/ could easily get confused with the Wayland protocol .xml files.

Still, I think that's a minor confusion that will quickly go away when someone looks inside the directory, as opposed to something that has to be explained to people before it makes any sense.

Review of attachment 299282 [details] [review]:

If the point of the refactoring is to get to a clean design, there are elements of this patch that feel off to me

::: src/frontends/meta-cursor.c
@@ +156,2 @@
 MetaCursorSprite *
+meta_cursor_sprite_new (void)

meta_foo_new() should create a MetaFoo, not a MetaFoo or a subclass. This is something like meta_create_cursor_sprite().

@@ +160,3 @@
+    return g_object_new (META_TYPE_CURSOR_SPRITE_WAYLAND, NULL);
+  else
+    return g_object_new (META_TYPE_CURSOR_SPRITE, NULL);

Hmm, I think this reflects confusion whether MetaCursorSprite is a frontend or a backend object. If it's genuinely a "frontend" object, then when creating it we should always know what protocol we are creating it for. And meta_cursor_sprite_from_theme() would be a third subclass independent of the backend.

Obviously every transformation of frontend to backend isn't going to be possible - e.g., creating an X cursor for a Wayland surface - but I don't think that's a big problem - you just need to have the code somewhere that knows what can be transformed to what.

::: src/frontends/wayland/meta-cursor-wayland.c
@@ +66,3 @@
+  MetaCursorSpriteWayland *self;
+
+  self = META_CURSOR_SPRITE_WAYLAND (meta_cursor_sprite_new ());

This doesn't make sense to me - this *has* to be a MetaCursorSpriteWayland, so just create one.

Review of attachment 301060 [details] [review]:

Hmm, this deviates a lot from how I'd expect things to be organized. Use your judgement whether you agree with me or not :-) If you want me to review things as is without any reconceptualization of MetaCursorSprite , let me know, and I'll do that.

::: src/backends/meta-cursor-renderer.c
@@ -103,3 @@
-      priv->current_rect.y = priv->current_y - hot_y;
-      priv->current_rect.width = cogl_texture_get_width (COGL_TEXTURE (texture));
-      priv->current_rect.height = cogl_texture_get_height (COGL_TEXTURE (texture));

I'd have expected the rectangle computations to stay here and the MetaCursorSprite operation to be more like "here's the desired scale, what do you have for me?"

::: src/frontends/wayland/meta-cursor-wayland.c
@@ +129,3 @@
+      image_scale = 1;
+      if (priv->monitor_scale != monitor->scale)
+        meta_cursor_sprite_load_from_theme (cursor_sprite, monitor->scale);

This is certainly not where I'd expect to find this code... in the pure "frontend" conceptualization of MetaCursorSprite, a MetaCursorSpriteWayland has nothing to do with themed cursors - a wayland cursor is one set by a Wayland client.

(In reply to Owen Taylor from comment #29)
> Review of attachment 299281 [details] [review] [review]:
> 
> Hmm, in the context of trying to get 3.16.x working well in a hidpi
> environment, this seems like it's a bit of an extraneous refactor. 
> 
> I dislike the frontends/ naming - I think it's "clever" rather than
> informative rather than informative. There's a pretty well established idea
> that the "frontend" of a system is on the user-facing side
> (http://en.wikipedia.org/wiki/Front_and_back_ends), so I think people are
> not going to look in a frontends/ directory for the code relating to
> protocols. I think this would be better called protocols/.
> 
> From IRC <Jasper> And protocol/ could easily get confused with the Wayland
> protocol .xml files.
> 
> Still, I think that's a minor confusion that will quickly go away when
> someone looks inside the directory, as opposed to something that has to be
> explained to people before it makes any sense.

I suppose protocol/ works as well, but I have some kind of plan to split x11 into x11/cm and x11/xwayland. Is protocol/ a still good name for that you think?

(In reply to Owen Taylor from comment #28)
> So this is a pretty big patch set - so I wanted to stop back and discuss
> where we want to go with this for 3.16.
> 
>  * Is there some simpler patch that just fixes the initial bug report and
> gets us to the point where we aren't regressing from X and using the wrong
> cursor size when there's only one monitor? It looks like GTK+ tries to
> create a cursor of the right size. What if we just used the cursor set by
> the application pixel-for-pixel ignoring the scale set on it? (Or are we
> already doing that and the problem is the lack of proper enter/leave events?)
> 
>  * If we want to land this full set of patches, what I think we should do is
> wait until 3.16.1 (which is Monday, I think anyways?) and mutter is
> branched, and land them on master, and then look at backporting once they've
> had a little chance to get some testing and mature.

I suppose we could just always load bigger version of the theme in that case. I can look into that on Monday.

(In reply to Owen Taylor from comment #30)
> Review of attachment 299282 [details] [review] [review]:
> 
> If the point of the refactoring is to get to a clean design, there are
> elements of this patch that feel off to me
> 
> ::: src/frontends/meta-cursor.c
> @@ +156,2 @@
>  MetaCursorSprite *
> +meta_cursor_sprite_new (void)
> 
> meta_foo_new() should create a MetaFoo, not a MetaFoo or a subclass. This is
> something like meta_create_cursor_sprite().

Then meta_create_cursor_sprite() it is then.

> 
> @@ +160,3 @@
> +    return g_object_new (META_TYPE_CURSOR_SPRITE_WAYLAND, NULL);
> +  else
> +    return g_object_new (META_TYPE_CURSOR_SPRITE, NULL);
> 
> Hmm, I think this reflects confusion whether MetaCursorSprite is a frontend
> or a backend object. If it's genuinely a "frontend" object, then when
> creating it we should always know what protocol we are creating it for. And
> meta_cursor_sprite_from_theme() would be a third subclass independent of the
> backend.

MetaCursorSprite is a kind of a frontend/protocol thing. Some confusion is that while we always have one backend, we have multiple frontends/protocols but one "running mode" (i.e. Wayland compositor or X11 CM), so really MetaCursorSprite is always a MetaCursorSpriteWayland when running as a Wayland compositor. I've been thinking of organising things in some way that reflects that but haven't come up with any good idea yet.

> 
> Obviously every transformation of frontend to backend isn't going to be
> possible - e.g., creating an X cursor for a Wayland surface - but I don't
> think that's a big problem - you just need to have the code somewhere that
> knows what can be transformed to what.
> 
> ::: src/frontends/wayland/meta-cursor-wayland.c
> @@ +66,3 @@
> +  MetaCursorSpriteWayland *self;
> +
> +  self = META_CURSOR_SPRITE_WAYLAND (meta_cursor_sprite_new ());
> 
> This doesn't make sense to me - this *has* to be a MetaCursorSpriteWayland,
> so just create one.

The idea was to centralize creation of a MetaCursorSprite* but I can remove that centralization if you prefer.(In reply to Owen Taylor from comment #31)

> Review of attachment 301060 [details] [review] [review]:
> 
> Hmm, this deviates a lot from how I'd expect things to be organized. Use
> your judgement whether you agree with me or not :-) If you want me to review
> things as is without any reconceptualization of MetaCursorSprite , let me
> know, and I'll do that.
> 
> ::: src/backends/meta-cursor-renderer.c
> @@ -103,3 @@
> -      priv->current_rect.y = priv->current_y - hot_y;
> -      priv->current_rect.width = cogl_texture_get_width (COGL_TEXTURE
> (texture));
> -      priv->current_rect.height = cogl_texture_get_height (COGL_TEXTURE
> (texture));
> 
> I'd have expected the rectangle computations to stay here and the
> MetaCursorSprite operation to be more like "here's the desired scale, what
> do you have for me?"

The resulting rectangle is not only used by the renderer, it is also used to detect what outputs the cursor wl_surface is on (see meta-wayland-surface.c in a later patch). In other words, since MetaCursorSpriteWayland sometimes can be a wl_surface, we need to treat it more as such.

> 
> ::: src/frontends/wayland/meta-cursor-wayland.c
> @@ +129,3 @@
> +      image_scale = 1;
> +      if (priv->monitor_scale != monitor->scale)
> +        meta_cursor_sprite_load_from_theme (cursor_sprite, monitor->scale);
> 
> This is certainly not where I'd expect to find this code... in the pure
> "frontend" conceptualization of MetaCursorSprite, a MetaCursorSpriteWayland
> has nothing to do with themed cursors - a wayland cursor is one set by a
> Wayland client.

A MetaCursorSpriteWayland is the MetaCursorSprite implementation when running as a Wayland compositor. Again, this confusion is probably that we don't have a "running mode" abstraction, only the "meta_is_wayland_compositor ()" thing which is not very nice.

Created attachment 301435 [details] [review]
wayland: Always scale cursor according to 'scaling-factor'

Always scale pointer cursors with the 'scaling-factor' gsetting. For
theme based cursos this means loading a larger version of the theme, and
for wl_buffer based cursors this means taking the buffer scale into
account and potentially scaling the texture accordingly.


---

This patch simply does what the commit message does. I have tested this by changing the 'scaling-factor' setting on my non-HiDPI hardware, but that is all the testing I have done so far. Is this simple enough for the 3.16 branch? If we'd go with something like this, I think it'd be best to push this after branching.

As I have never seen how things work on HiDPI hardware I'm not sure its the right way (do theme_size * scaling_factor), so I'd appreciate some insight into that as well.

Note that GTK+ clients will show blurry cursors (at least for me, by just changing the 'scaling-factor' on my non-HiDPI hardware) since it doesn't know what outputs its surfaces are on.

(In reply to Jonas Ådahl from comment #32)
> (In reply to Owen Taylor from comment #29)
> > Review of attachment 299281 [details] [review] [review] [review]:
> > 
> > Hmm, in the context of trying to get 3.16.x working well in a hidpi
> > environment, this seems like it's a bit of an extraneous refactor. 
> > 
> > I dislike the frontends/ naming - I think it's "clever" rather than
> > informative rather than informative. There's a pretty well established idea
> > that the "frontend" of a system is on the user-facing side
> > (http://en.wikipedia.org/wiki/Front_and_back_ends), so I think people are
> > not going to look in a frontends/ directory for the code relating to
> > protocols. I think this would be better called protocols/.
> > 
> > From IRC <Jasper> And protocol/ could easily get confused with the Wayland
> > protocol .xml files.
> > 
> > Still, I think that's a minor confusion that will quickly go away when
> > someone looks inside the directory, as opposed to something that has to be
> > explained to people before it makes any sense.
> 
> I suppose protocol/ works as well, but I have some kind of plan to split x11
> into x11/cm and x11/xwayland. Is protocol/ a still good name for that you
> think?

There are two possibilities:

 A) The code in x11/cm is specifically code that is particularly about interacting with X11 clients as a compositor - perhaps the frame sync code. In this case, protocol/x11/cm seems fine.
 B) The code in x11/cm is just general code that is specific to being an X11 compositor without wayland. In this case, being in a directory conceptualized as protocol/frontend code is wrong.

The acid test here is can you write down a coherent short description of what goes where, and does the code you are putting in that location actually consistently fit that pattern? Can you try writing that description and see if the cursor code fits it?

(winsys/ would be a very general name to cover code about a windowing system, and perhaps it's not *too* confusing to have both winsys/x11 and backends/x11.)

(In reply to Jonas Ådahl from comment #34)
> (In reply to Owen Taylor from comment #30)
> > Review of attachment 299282 [details] [review] [review] [review]:
> > 
> > If the point of the refactoring is to get to a clean design, there are
> > elements of this patch that feel off to me
> > 
> > ::: src/frontends/meta-cursor.c
> > @@ +156,2 @@
> >  MetaCursorSprite *
> > +meta_cursor_sprite_new (void)
> > 
> > meta_foo_new() should create a MetaFoo, not a MetaFoo or a subclass. This is
> > something like meta_create_cursor_sprite().
> 
> Then meta_create_cursor_sprite() it is then.
> 
> > 
> > @@ +160,3 @@
> > +    return g_object_new (META_TYPE_CURSOR_SPRITE_WAYLAND, NULL);
> > +  else
> > +    return g_object_new (META_TYPE_CURSOR_SPRITE, NULL);
> > 
> > Hmm, I think this reflects confusion whether MetaCursorSprite is a frontend
> > or a backend object. If it's genuinely a "frontend" object, then when
> > creating it we should always know what protocol we are creating it for. And
> > meta_cursor_sprite_from_theme() would be a third subclass independent of the
> > backend.
> 
> MetaCursorSprite is a kind of a frontend/protocol thing. Some confusion is
> that while we always have one backend, we have multiple frontends/protocols
> but one "running mode" (i.e. Wayland compositor or X11 CM), so really
> MetaCursorSprite is always a MetaCursorSpriteWayland when running as a
> Wayland compositor. I've been thinking of organising things in some way that
> reflects that but haven't come up with any good idea yet.

So:

 Wayland running mode:
  - With *both* wayland and X protocols to talk to clients
  - With *either* native or x11 backend

 X11 compositor:
  - With X protocol to talk to client
  - With x11 backend
  
Code might be X11 code because it's about a window that we are going to act as a X11 window manager for (X window or Xwayland window), it might be X11 code because it's handling X input events (X compositor or nested Wayland), it might be X11 code because it's specific to when we are running as a X compositor. This is obviously pretty hard to represent either with a directory hierarchy or with a class hierarchy.

I think trying to be too strict with how we organize things is going to be confusing, which could encourage something like a winsys/ directory (I guess this is the same as our current x11/ and wayland/ directories in the toplevel.) I'm not sure I believe in a strict "frontend protocol" directory, because it seems like the first thing that you wanted to put in there doesn't really match that directory!

That all being said I have some specific concerns about MetaCursorSprite vs. MetaCursorSpriteWayland.

MetaCursorSprite, as I understand it, is basically similar to  MetaCursorReference - it's basically a union of different ways to specify the appearance of a cursor. A wayland-specific subclass of this might make sense so that it can store along with the appearance a pointer back to a Wayland object. But a Wayland specific subclass shouldn't gain entirely new capabilities like looking going out and looking at where it is relative to the monitors and reloading the theme image at a different size.

I feel like the parent class is fur, and the subclass is a cat!

Things to think about for improving the design:

 - Do some pieces (like loading from the theme at a different size) work better in the generic code, even if they are only ever used in the Wayland case?
 - Can meta_cursor_sprite_wayland_update_position() be modified to use only public functionality and then live somewhere else in the code, perhaps as a procedural method?

As I said before, if you think this is the best way to organize it, I'm willing to just review it as is - sometimes there's no clean design that actually works out.

The frontend needs a way of specifying "a cursor" (a wl_surface, an X cursor name), and the backend needs a way of accepting that and saying "I prefer a wl_surface" or "I prefer an X cursor name", because accepting the other might be costly.

There's conceptually two separate objects, one frontend (the protocol object the user manipulates) and one backend (the object that is being displayed on the screen), but in practice they're not different enough to be two different objects. That's why it's sort of in this weird middle-ground right now.

(In reply to Owen Taylor from comment #37)
> (In reply to Jonas Ådahl from comment #34)
> > (In reply to Owen Taylor from comment #30)
> > > Review of attachment 299282 [details] [review] [review] [review] [review]:
> > > 
> > > If the point of the refactoring is to get to a clean design, there are
> > > elements of this patch that feel off to me
> > > 
> > > ::: src/frontends/meta-cursor.c
> > > @@ +156,2 @@
> > >  MetaCursorSprite *
> > > +meta_cursor_sprite_new (void)
> > > 
> > > meta_foo_new() should create a MetaFoo, not a MetaFoo or a subclass. This is
> > > something like meta_create_cursor_sprite().
> > 
> > Then meta_create_cursor_sprite() it is then.
> > 
> > > 
> > > @@ +160,3 @@
> > > +    return g_object_new (META_TYPE_CURSOR_SPRITE_WAYLAND, NULL);
> > > +  else
> > > +    return g_object_new (META_TYPE_CURSOR_SPRITE, NULL);
> > > 
> > > Hmm, I think this reflects confusion whether MetaCursorSprite is a frontend
> > > or a backend object. If it's genuinely a "frontend" object, then when
> > > creating it we should always know what protocol we are creating it for. And
> > > meta_cursor_sprite_from_theme() would be a third subclass independent of the
> > > backend.
> > 
> > MetaCursorSprite is a kind of a frontend/protocol thing. Some confusion is
> > that while we always have one backend, we have multiple frontends/protocols
> > but one "running mode" (i.e. Wayland compositor or X11 CM), so really
> > MetaCursorSprite is always a MetaCursorSpriteWayland when running as a
> > Wayland compositor. I've been thinking of organising things in some way that
> > reflects that but haven't come up with any good idea yet.
> 
> So:
> 
>  Wayland running mode:
>   - With *both* wayland and X protocols to talk to clients
>   - With *either* native or x11 backend

Note that the x11 backend here behaves differently depending on the running mode which is why I have been suggesting backends/x11/[cm/nested].

> 
>  X11 compositor:
>   - With X protocol to talk to client
>   - With x11 backend
>   
> Code might be X11 code because it's about a window that we are going to act
> as a X11 window manager for (X window or Xwayland window), it might be X11
> code because it's handling X input events (X compositor or nested Wayland),
> it might be X11 code because it's specific to when we are running as a X
> compositor. This is obviously pretty hard to represent either with a
> directory hierarchy or with a class hierarchy.

We have already started separating backends from the rest, so I think in that case the abstraction can be pretty straight forward (see my comment above).

> 
> I think trying to be too strict with how we organize things is going to be
> confusing, which could encourage something like a winsys/ directory (I guess
> this is the same as our current x11/ and wayland/ directories in the
> toplevel.) I'm not sure I believe in a strict "frontend protocol" directory,
> because it seems like the first thing that you wanted to put in there
> doesn't really match that directory!
> 

I don't think "winsys" is good term for it either. We'd have different "winsys" meaning different things depending on the layer (clutter/cogl vs mutter). The reason MetaCursorSpriteWayland is an awkward fit is because it's a mix of Wayland client interaction logic and running mode logic. So maybe a better design would be to split those into different units.

> That all being said I have some specific concerns about MetaCursorSprite vs.
> MetaCursorSpriteWayland.
> 
> MetaCursorSprite, as I understand it, is basically similar to 
> MetaCursorReference - it's basically a union of different ways to specify
> the appearance of a cursor. A wayland-specific subclass of this might make
> sense so that it can store along with the appearance a pointer back to a
> Wayland object. But a Wayland specific subclass shouldn't gain entirely new
> capabilities like looking going out and looking at where it is relative to
> the monitors and reloading the theme image at a different size.
> 
> I feel like the parent class is fur, and the subclass is a cat!

That might be because when an X CM, the cat lives in the X server, but in Wayland we are the cat.

> 
> Things to think about for improving the design:
> 
>  - Do some pieces (like loading from the theme at a different size) work
> better in the generic code, even if they are only ever used in the Wayland
> case?

This is why I put it in the Wayland object; it only ever makes sense do do it there.

>  - Can meta_cursor_sprite_wayland_update_position() be modified to use only
> public functionality and then live somewhere else in the code, perhaps as a
> procedural method?
> 
> As I said before, if you think this is the best way to organize it, I'm
> willing to just review it as is - sometimes there's no clean design that
> actually works out.

I'd have to think about this more. I agree its not an optimal design as it blurs the line between "running mode" and the protocol used.

(In reply to Jasper St. Pierre from comment #38)
> The frontend needs a way of specifying "a cursor" (a wl_surface, an X cursor
> name), and the backend needs a way of accepting that and saying "I prefer a
> wl_surface" or "I prefer an X cursor name", because accepting the other
> might be costly.
> 
> There's conceptually two separate objects, one frontend (the protocol object
> the user manipulates) and one backend (the object that is being displayed on
> the screen), but in practice they're not different enough to be two
> different objects. That's why it's sort of in this weird middle-ground right
> now.

I already separated the backend from the frontend completely (all backend related code is now in MetaCursorRenderer*).

Its rather "meta_is_wayland_compositor ()" or "running mode" or whatever to call it.

Maybe we should have such a abstraction, not calling it Wayland but "display server", and then hook wl_surface etc to it some other way and put it under a "protocols/" directory. Not sure what to call that though; its neither "winsys", "protocol" or "frontend".

I.e. something like

backends/
  x11/
    cm/
    nested/
protocols/
  wayland/
  x11/
  xwayland/ (funky mix of the two above)
"running mode"/
  x11_cm/
  display_server/

?

MetaCursorSprite would go under "running mode"/ and under protocols we'd have a MetaWaylandCursor which interacts with MetaCursorSprite.

I consider nested X11 a developer tool only, so we haven't given it the real attention and split it out to a "true" backend. Things like barriers and input settings really shouldn't be running as nested X11 either.

Note that I already have an enum here for what you call "running mode" to cut down on the "meta_is_wayland_compositor" in the backend code.

https://git.gnome.org/browse/mutter/tree/src/backends/x11/meta-backend-x11.c#n49

But personally, I don't care enough about the nested code -- it's not production, it's development and testing only, and as long as it's not too messy, let's not overcomplicate things. I'm fine with having nested not use a X11 cursor and instead always draw it ourselves, for instance. It's why we call XFixesHideCursor even in nested mode. If you don't like that you don't have a cursor sometimes, move your mouse back over the nested window enough that it shows again, and then alt-tab to another window.

If you feel really encouraged to fix this, feel free, but I've just not bothered.

(In reply to Jasper St. Pierre from comment #40)
> I consider nested X11 a developer tool only, so we haven't given it the real
> attention and split it out to a "true" backend. Things like barriers and
> input settings really shouldn't be running as nested X11 either.
> 

Yea, with the backends/x11/(nested|cm) I only mean to move out Dummy things out of the way, and the is_wayland_compositor/X11_MODE as below to be in their files. Anyhow it's the least important of the abstractions here, since it isn't all that bad.

> Note that I already have an enum here for what you call "running mode" to
> cut down on the "meta_is_wayland_compositor" in the backend code.
> 
> https://git.gnome.org/browse/mutter/tree/src/backends/x11/meta-backend-x11.
> c#n49

What I mean when I say running mode is not really related to backends.

> 
> But personally, I don't care enough about the nested code -- it's not
> production, it's development and testing only, and as long as it's not too
> messy, let's not overcomplicate things. I'm fine with having nested not use
> a X11 cursor and instead always draw it ourselves, for instance. It's why we
> call XFixesHideCursor even in nested mode. If you don't like that you don't
> have a cursor sometimes, move your mouse back over the nested window enough
> that it shows again, and then alt-tab to another window.

I have patches for that (lost cursor issue), but they depend on these patches so I haven't pushed them yet.

> 
> If you feel really encouraged to fix this, feel free, but I've just not
> bothered.

That is what these patches tries to do, or at least now we are discussing how to do it better. A MetaCursorSprite behaves differently depending on whether we are a CM or a DS, and there is logic related to the client type.

Owen was not particularly fond of me putting so much things that are just related to being a DS (being able to scale, load different theme sizes; things irrelevant for a CM) inside a protocols/wayland abstraction and that is why I've been suggesting another abstraction layer, i.e. the "running mode", and putting those parts there, and then the Wayland related parts elsewhere.

There are probably other things that could end up behind such an abstraction, for example some code that is behind (!)meta_is_wayland_compositor in compositor.c, display.c, screen.c etc.

(In reply to Jonas Ådahl from comment #39)
> > That all being said I have some specific concerns about MetaCursorSprite vs.
> > MetaCursorSpriteWayland.
> > 
> > MetaCursorSprite, as I understand it, is basically similar to 
> > MetaCursorReference - it's basically a union of different ways to specify
> > the appearance of a cursor. A wayland-specific subclass of this might make
> > sense so that it can store along with the appearance a pointer back to a
> > Wayland object. But a Wayland specific subclass shouldn't gain entirely new
> > capabilities like looking going out and looking at where it is relative to
> > the monitors and reloading the theme image at a different size.
> > 
> > I feel like the parent class is fur, and the subclass is a cat!
> 
> That might be because when an X CM, the cat lives in the X server, but in
> Wayland we are the cat.

I don't want to push metaphors too hard, but if we think that in the "X CM" case, the cat is in the X server, then in that case, our object should still be a cat, even if it's a remote control cat-shaped toy.

> > 
> > Things to think about for improving the design:
> > 
> >  - Do some pieces (like loading from the theme at a different size) work
> > better in the generic code, even if they are only ever used in the Wayland
> > case?
> 
> This is why I put it in the Wayland object; it only ever makes sense do do
> it there.

My suggestion is the opposite - sometimes it makes sense to do things in the base class *even if they are only ever used in one subclass* - this can enhance encapsulation, avoid unnecessary virtualization, or just make things make more sense.

[...]

> Maybe we should have such a abstraction, not calling it Wayland but "display
> server", and then hook wl_surface etc to it some other way and put it under
> a "protocols/" directory. Not sure what to call that though; its neither
> "winsys", "protocol" or "frontend".
> 
> I.e. something like
> 
> backends/
>   x11/
>     cm/
>     nested/
> protocols/
>   wayland/
>   x11/
>   xwayland/ (funky mix of the two above)
> "running mode"/
>   x11_cm/
>   display_server/
> 
> ?
> 
> MetaCursorSprite would go under "running mode"/ and under protocols we'd
> have a MetaWaylandCursor which interacts with MetaCursorSprite.

Hmm, I think the directory organization is getting ahead of the code organization. How do we replace the cases where we have meta_is_wayland_compositor() conditionals? How do we move x11 and wayland specific code out of common source files in core/ and compositor/? Having nice clean subdirectories doesn't make sense if we can't move the relevant code there.

Created attachment 307713 [details] [review]
Make MetaCursorSprite a GObject

To easier track lifetime and utilize other GObject features, make
MetaCursorSprite a GObject.

Created attachment 307714 [details] [review]
backends: Get rid of meta-cursor-private.h

There is nothing special about the private API which only consists of
getters for renderer specific backing buffer. Lets them to the regular
.h file and treat them as part of the normal API.

Created attachment 307715 [details] [review]
MetaCursorSprite: Put renderer specific code in the renderer

There were lots of code handling the native renderer specific cases;
move these parts to the renderer. Note that this causes the X11 case to
always generate the texture which is a waste of memory, but his
regression will be fixed in a following commit.

Created attachment 307716 [details] [review]
MetaWaylandSurface: Make it a GObject

This way we can add signals and weak references without relying on
wl_signal, wl_listener etc.

Created attachment 307717 [details] [review]
wayland: GObject:ify surface roles

Make a surface roles into objects with vfuncs for things where there
before was a big switch statement. The declaration and definition
boilerplate is hidden behind C macros.

Created attachment 307718 [details] [review]
Move meta-cursor* from backends/ to core/

They are no longer related to the backend, and we don't have a better
place to put it yet, so put it in core/.

Created attachment 307719 [details] [review]
backends/x11: Draw our own cursor sprite when running nested

Use a specialized cursor renderer when running as a nested Wayand
compositor. This new renderer sets an empty X11 cursor and draws the
cursor as part of the stage using the generic cursor renderer drawing
path.

Created attachment 307720 [details] [review]
MetaCursorRenderer: Use G_DECLARE_DERIVABLE_TYPE to declare the type

Created attachment 307721 [details] [review]
wayland: Keep what surface a surface role is associated with

Will be useful in later commits.

Created attachment 307722 [details] [review]
MetaCursorSprite: Squash MetaCurorImage into MetaCursorSprite

It fills little purpose on separating into a MetaCursorImage struct, so
lets squash in the three fields into the MetaCursorSprite object.

Created attachment 307723 [details] [review]
MetaWaylandPointer: Don't keep our own MetaCursorTracker pointer

There is no reason to, we can just retrieve it every time we need it.

Created attachment 307724 [details] [review]
wayland: Move cursor surface role to meta-wayland-pointer.c

The wl_pointer assigns a role to a wl_surface, so it makes sense to put
the related logic there.

Created attachment 307725 [details] [review]
Support scaling of cursor sprites given what output they are on

This commits refactors cursor handling code and plugs in logic so that
cursor sprites changes appearance as it moves across the screen.
Renderers are adapted to handle the necessary functionality.

The logic for changing the cursor sprite appearance is done outside of
MetaCursorSprite, and actually where depends on what type of cursor it
is. In mutter we now have two types of cursors that may have their
appearance changed:

 * Themed cursors (aka root cursors)
 * wl_surface cursors

Themed cursors are created by MetaScreen and when created, when
applicable(*), it will extend the cursor via connecting to a signal
which is emitted everytime the cursor is moved. The signal handler will
calculate the expected scale given the monitor it is on and reload the
theme in a correct size when needed.

wl_surface cursors are created when a wl_surface is assigned the
"cursor" role, i.e. when a client calls wl_pointer.set_cursor. A
cursor role object is created which is connected to the cursor object
by the position signal, and will set a correct texture scale given what
monitor the cursor is on and what scale the wl_surface's active buffer
is in. It will also push new buffers to the same to the cursor object
when new ones are committed to the surface.

* when we are running as a Wayland compositor

Created attachment 307726 [details] [review]
wayland: Support sending wl_surface.enter/leave to cursor surfaces

Support notifying clients about what outputs their cursor surfaces are
on so that they can attach appropriately scaled buffers to them.

Review of attachment 307719 [details] [review]:

Considering I originally wrote the X11 cursor renderer with the goal of nested in mind, I'm not exactly sure what the point is here.

Review of attachment 307713 [details] [review]:

OK.

Review of attachment 307714 [details] [review]:

The original goal here was that nothing but the renderers should be able to poke into the struct internals, but I'm OK with this.

Review of attachment 307715 [details] [review]:

I like this.

Review of attachment 307717 [details] [review]:

Not at all a fan of this. Instead of a GObject, perhaps just use a regular struct of vfuncs?

Review of attachment 307718 [details] [review]:

Nah. core/ is for core window management logic. Just leave it in backends/, please.

Review of attachment 307722 [details] [review]:

OK.

Review of attachment 307715 [details] [review]:

Actually, it says "this causes the X11 case to always generate the texture, will be fixed in a following commit", but I don't see any place where you actually fix this...

Review of attachment 307720 [details] [review]:

Seems simple enough, thanks. Feel free to commit before anything else.

Review of attachment 307723 [details] [review]:

There was originally a reason when there was multi-screen support, but that doesn't matter anymore. Nice cleanup, thanks.

Review of attachment 307716 [details] [review]:

OK.

(In reply to Jasper St. Pierre from comment #62)
> Review of attachment 307718 [details] [review] [review]:
> 
> Nah. core/ is for core window management logic. Just leave it in backends/,
> please.

Well its not part of the backend either, but fine.

(In reply to Jasper St. Pierre from comment #64)
> Review of attachment 307715 [details] [review] [review]:
> 
> Actually, it says "this causes the X11 case to always generate the texture,
> will be fixed in a following commit", but I don't see any place where you
> actually fix this...

It happens in the huge (second last) patch. The texture is loaded from "meta_cursor_sprite_realize_texture() which is called by the renderer when it knows it needs it.

(In reply to Jasper St. Pierre from comment #61)
> Review of attachment 307717 [details] [review] [review]:
> 
> Not at all a fan of this. Instead of a GObject, perhaps just use a regular
> struct of vfuncs?

In the second last patch I put state in the cursor role object and I also g_signal_connect_object it. The signal I suppose can be connected to the surface object, but then I'd still have to have a data type that has instance variables, vfuncs and a destroy vfunc, which means I'd end up with special purpose GObject-mini type. Just using GObject felt more reasonable because of that.

I have also had ideas about moving out all the window logic from MetaWaylandSurface to a window role or similar, and then it might be nice to actually be a GObject.

(In reply to Jasper St. Pierre from comment #57)
> Review of attachment 307719 [details] [review] [review]:
> 
> Considering I originally wrote the X11 cursor renderer with the goal of
> nested in mind, I'm not exactly sure what the point is here.

With cursors having different themes depending on what (nested) output it is on, it felt more close to reality to just always draw it as a texture instead of hiding and showing the server cursor. It felt cleaner to just have a separate renderer for the nested case since it'd share none of the functionality.

Comment on attachment 307720 [details] [review]
MetaCursorRenderer: Use G_DECLARE_DERIVABLE_TYPE to declare the type

Attachment 307720 [details] pushed as 975feb9 - MetaCursorRenderer: Use G_DECLARE_DERIVABLE_TYPE to declare the type

Created attachment 309526 [details] [review]
Rename MetaCursorReference to MetaCursorSprite

It isn't really a reference to a MetaCursor enum, but a reference
counted cursor sprite, and enum value.

Created attachment 309527 [details] [review]
Make MetaCursorSprite a GObject

To easier track lifetime and utilize other GObject features, make
MetaCursorSprite a GObject.

Created attachment 309528 [details] [review]
MetaWaylandSurface: Make it a GObject

This way we can add signals and weak references without relying on
wl_signal, wl_listener etc.

Created attachment 309529 [details] [review]
MetaWaylandPointer: Don't keep our own MetaCursorTracker pointer

There is no reason to, we can just retrieve it every time we need it.

Created attachment 309530 [details] [review]
backends: Get rid of meta-cursor-private.h

There is nothing special about the private API which only consists of
getters for renderer specific backing buffer. Lets them to the regular
.h file and treat them as part of the normal API.

Created attachment 309531 [details] [review]
MetaCursorSprite: Put renderer specific code in the renderer

There were lots of code handling the native renderer specific cases;
move these parts to the renderer. Note that this causes the X11 case to
always generate the texture which is a waste of memory, but his
regression will be fixed in a following commit.

The lazy loading of the texture was removed because it was eventually
always loaded anyway indirectly by the renderer to calculate the
current rect.

Created attachment 309532 [details] [review]
MetaWaylandSurface: Don't respond to frame callback when role unassigned

We won't draw a unassigned surface, so don't fool the client that it was
drawed.

Created attachment 309533 [details] [review]
wayland: GObject:ify surface roles

Make a surface roles into objects with vfuncs for things where there
before was a big switch statement. The declaration and definition
boilerplate is hidden behind C macros.

Created attachment 309534 [details] [review]
backends/x11: Draw our own cursor sprite when running nested

Use a specialized cursor renderer when running as a nested Wayand
compositor. This new renderer sets an empty X11 cursor and draws the
cursor as part of the stage using the generic cursor renderer drawing
path.

Created attachment 309535 [details] [review]
wayland: Keep what surface a surface role is associated with

Will be useful in later commits.

Created attachment 309536 [details] [review]
MetaCursorSprite: Squash MetaCurorImage into MetaCursorSprite

It fills little purpose on separating into a MetaCursorImage struct, so
lets squash in the three fields into the MetaCursorSprite object.

Created attachment 309537 [details] [review]
wayland: Move cursor surface role to meta-wayland-pointer.c

The wl_pointer assigns a role to a wl_surface, so it makes sense to put
the related logic there.

Created attachment 309538 [details] [review]
Support scaling of cursor sprites given what output they are on

This commits refactors cursor handling code and plugs in logic so that
cursor sprites changes appearance as it moves across the screen.
Renderers are adapted to handle the necessary functionality.

The logic for changing the cursor sprite appearance is done outside of
MetaCursorSprite, and actually where depends on what type of cursor it
is. In mutter we now have two types of cursors that may have their
appearance changed:

 - Themed cursors (aka root cursors)
 - wl_surface cursors

Themed cursors are created by MetaScreen and when created, when
applicable(*), it will extend the cursor via connecting to a signal
which is emitted everytime the cursor is moved. The signal handler will
calculate the expected scale given the monitor it is on and reload the
theme in a correct size when needed.

wl_surface cursors are created when a wl_surface is assigned the
"cursor" role, i.e. when a client calls wl_pointer.set_cursor. A
cursor role object is created which is connected to the cursor object
by the position signal, and will set a correct texture scale given what
monitor the cursor is on and what scale the wl_surface's active buffer
is in. It will also push new buffers to the same to the cursor object
when new ones are committed to the surface.

This commit also makes texture loading lazy, since the renderer doesn't
calculate a rectangle when the cursor position changes.

* when we are running as a Wayland compositor

Created attachment 309539 [details] [review]
wayland: Support sending wl_surface.enter/leave to cursor surfaces

Support notifying clients about what outputs their cursor surfaces are
on so that they can attach appropriately scaled buffers to them.

Created attachment 309540 [details] [review]
MetaCursorRenderer: Rely on update_cursor for redrawing

Calling queue_redraw() in _force_update() is not needed because
update_cursor() will do this when needed, i.e. when switching between
hardware cursor and texture cursor, or when drawing with texture cursor.

There is also no need to force _native_force_update() because
update_cursor() will cover this as well when needed. When not changing
cursor but only the gbm_bo, the "dirty" boolean on the gbm_bo will
trigger a redraw.

The newly uploaded series is a rebase on top of current master with various issues (the ones were a consensus was reached) addressed. It is not a trivial rebase, since some patches needed to be adapted the animated themed cursors changes.

If no-one objects, to decrease the amount of time I spend rebasing patches, I'll go ahead and push the first four patches, since they got the green flag last round.

A more up to date branch can be found here: https://github.com/jadahl/mutter/commits/wip/hidpi-cursor

I'm not uploading newer versions here because I suspect the patch order will be messed up. The changes since the patches attached here the new "wayland: Introduce XWayland surface role" patch and the updated "MetaWaylandSurface: Don't respond to frame callback when role unassigned" patch (and corresponding updates to the later patches).

The branch could probably be split in two - one for the surface roles and then for the hidpi cursors part. I can report the roles part as a separate bug if anyone would prefer.

(In reply to Jonas Ådahl from comment #86)
> A more up to date branch can be found here:
> https://github.com/jadahl/mutter/commits/wip/hidpi-cursor
> 
> I'm not uploading newer versions here because I suspect the patch order will
> be messed up. The changes since the patches attached here the new "wayland:
> Introduce XWayland surface role" patch and the updated "MetaWaylandSurface:
> Don't respond to frame callback when role unassigned" patch (and
> corresponding updates to the later patches).
> 
> The branch could probably be split in two - one for the surface roles and
> then for the hidpi cursors part. I can report the roles part as a separate
> bug if anyone would prefer.

Please leave the branch and this bug untouched for now! I know that we've been really slow at reviewing this, but I'm trying to get to it now, and it already has a really intimidating size and history that makes review challenging.

Review of attachment 309526 [details] [review]:

Was approved by Jasper.

Review of attachment 309527 [details] [review]:

Previously approved

Review of attachment 309528 [details] [review]:

Previously approved

Review of attachment 309529 [details] [review]:

Previously approved

Review of attachment 309530 [details] [review]:

Jasper was OK with it, looks good to me.

Review of attachment 309531 [details] [review]:

Verified that a later patch does seem to prevent the X11 case from generating the texture.

Created attachment 309854 [details] [review]
wayland: Introduce XWayland surface role

Being a "XWayland window" should be considered equivalent to a role,
even though it is not part of any protocol anywhere.

Review of attachment 309854 [details] [review]:

I attached this patch for review as part of this branch... but looking at it, I'd definitely like it if we can figure out a way to separate it - it looks entirely unrelated to the idea of the branch, and I'm not convinced by it as a patch yet. Is there some way we can land this branch without this>

 * It's not clear to me why Xwayland windows are a separate role... they seem to behave a lot like shell surfaces, and in a brief look it appears Xwayland calls wl_shell_get_shell_surface() on them - which by the protocol makes them have the shell surface role. I don't particularly like introducing the idea that we have internal pseudo-roles that are different from the roles of the Wayland protocol. If we want to special-case Xwayland windows, shouldn't we do it by some other method?

 * The commit message here doesn't reflect the idea of the patch at all, because looking at the huge comment in meta-wayland-surface.c, it looks like the *idea* of the patch is that we want to somehow treat frame callbacks differently on Xwayland windows to try and fix visual artifacts during map?

::: src/wayland/meta-xwayland.c
@@ +49,3 @@
+                                     META_WAYLAND_SURFACE_ROLE_XWAYLAND,
+                                     surface->resource,
+                                     WL_DISPLAY_ERROR_INVALID_OBJECT))

Hmmm ... I'm quite surprised that meta_wayland_surface_set_role() doesn't follow Mutter standards for error handling, but returns 0/-1... I'd definitely write this as:

 != 0

as other callers do to make it clearer what is going on.

(But please don't add more functions like this, and probably this one would be better changed.)

Review of attachment 309532 [details] [review]:

s/drawed/drawn/.

Commit message needs more motivation - what is going wrong that causes this patch to be needed?

::: src/wayland/meta-wayland-surface.c
@@ +550,3 @@
     case META_WAYLAND_SURFACE_ROLE_NONE:
+      wl_list_insert_list (&surface->frame_callback_list,
+                           &pending->frame_callback_list);

Needs a comment - like "since there is no role, we won't draw the window, so save the frame callbacks for later".

::: src/wayland/meta-wayland-surface.h
@@ +116,3 @@
+   * commit sequence, such as when it has not yet been assigned a role.
+   */
+  struct wl_list frame_callback_list;

should be 'pending_frame_callbacks' or something - if I see surface->frame_callback_list, I would have no expectation that it's a transient thing that is used just in some cases.

> and in a brief look it appears Xwayland calls wl_shell_get_shell_surface() on them

You're misreading the code. That's for the rootful case, where you run an X server *fully* nested under Wayland. We always pass -rootless.

(In reply to Owen Taylor from comment #95)
> Review of attachment 309854 [details] [review] [review]:
> 
> I attached this patch for review as part of this branch... but looking at
> it, I'd definitely like it if we can figure out a way to separate it - it
> looks entirely unrelated to the idea of the branch, and I'm not convinced by
> it as a patch yet. Is there some way we can land this branch without this>
> 
>  * It's not clear to me why Xwayland windows are a separate role... they
> seem to behave a lot like shell surfaces, and in a brief look it appears
> Xwayland calls wl_shell_get_shell_surface() on them - which by the protocol
> makes them have the shell surface role. I don't particularly like
> introducing the idea that we have internal pseudo-roles that are different
> from the roles of the Wayland protocol. If we want to special-case Xwayland
> windows, shouldn't we do it by some other method?

A "role" is meant to define what a wl_surface does. We already have a set of roles for all "ways" a wl_surface behaves - except XWayland wl_surfaces. We have the three different toplevel surface roles (wl_shell_surface, xdg_surface, xdg_popup), the subsurface role, cursor role and DND role. To me it simply only makes sense to introduce a XWayland role for wl_surface's that represent an XWindow, because they, like all the other roles, need to do things differently from all the other roles.

If we special case the XWayland "role" (or whatever to call it if we cant use that name in the implementation), then we'd need to have "if (have_role) do_role_specific_thing() else if (is_xwayland_window) do_xwayland_specific_thing()" which doesn't make sense at all to me.

I will insist we treat XWayland window wl_surface's as roles internally. If we'd had a wl_xwayland protocol, we could have a wl_xwayland_get_xwayland_surface that assigns such a role, but the code would look exactly the same in mutter for all other parts except when to assign the role. If you really don't want to call it role, we could s/role/something_else/ but I would still just assign something_else when the protocol defines we should assign a role.

I suppose one could say part of the problem is that XWayland - Wayland "protocol" is in no way specified, and we don't really have a place to specify the role-ness of how it works. I did contemplate whether it would make sense to have a wl_xwayland protocol just for the role assigning, but decided not to do that unless there are more things needed from such a protocol.

> 
>  * The commit message here doesn't reflect the idea of the patch at all,
> because looking at the huge comment in meta-wayland-surface.c, it looks like
> the *idea* of the patch is that we want to somehow treat frame callbacks
> differently on Xwayland windows to try and fix visual artifacts during map?

The huge comment just describes what was already done before. We kind of need to ignore the protocol here, we already do, and this patch just documents this and the reason why it is needed. The actual point of the patch is not really to describe reality though, but to introduce the role so we can deal with the wl_surface in the same way as with other wl_surface's.

> 
> ::: src/wayland/meta-xwayland.c
> @@ +49,3 @@
> +                                     META_WAYLAND_SURFACE_ROLE_XWAYLAND,
> +                                     surface->resource,
> +                                     WL_DISPLAY_ERROR_INVALID_OBJECT))
> 
> Hmmm ... I'm quite surprised that meta_wayland_surface_set_role() doesn't
> follow Mutter standards for error handling, but returns 0/-1... I'd
> definitely write this as:
> 
>  != 0
> 
> as other callers do to make it clearer what is going on.
> 
> (But please don't add more functions like this, and probably this one would
> be better changed.)

Sure. I wanted to differentiate it from an actual server side error, but I can make it an int().

(In reply to Jonas Ådahl from comment #98)
> (In reply to Owen Taylor from comment #95)
> > Review of attachment 309854 [details] [review] [review] [review]:
> > 
> > I attached this patch for review as part of this branch... but looking at
> > it, I'd definitely like it if we can figure out a way to separate it - it
> > looks entirely unrelated to the idea of the branch, and I'm not convinced by
> > it as a patch yet. Is there some way we can land this branch without this>

Indeed. I mentioned this in comment 86 (oh, many comments on this one now). In the branch I did this, except I put the 4 patches I planned to push earlier first (https://github.com/jadahl/mutter/commits/wip/hidpi-cursor is based on https://github.com/jadahl/mutter/commits/wip/surface-roles )

> > 
> > ::: src/wayland/meta-xwayland.c
> > @@ +49,3 @@
> > +                                     META_WAYLAND_SURFACE_ROLE_XWAYLAND,
> > +                                     surface->resource,
> > +                                     WL_DISPLAY_ERROR_INVALID_OBJECT))
> > 
> > Hmmm ... I'm quite surprised that meta_wayland_surface_set_role() doesn't
> > follow Mutter standards for error handling, but returns 0/-1... I'd
> > definitely write this as:
> > 
> >  != 0
> > 
> > as other callers do to make it clearer what is going on.
> > 
> > (But please don't add more functions like this, and probably this one would
> > be better changed.)
> 
> Sure. I wanted to differentiate it from an actual server side error, but I
> can make it an int().

I misread what you wrote, and misremembered what I wrote. This one is clearly just me forgetting to add the != 0 which I did in all the other set_role() places.

Review of attachment 309534 [details] [review]:

Makes sense to me ... the only advantage we get from using a server cursor is smoother updating on mouse motion, but the point of the nested backend is accurate testing, not ultimately smooth pointer updates - if there are lags, we'd like to see them.

Review of attachment 309534 [details] [review]:

Makes sense to me ... the only advantage we get from using a server cursor is smoother updating on mouse motion, but the point of the nested backend is accurate testing, not ultimately smooth pointer updates - if there are lags, we'd like to see them.

Review of attachment 309536 [details] [review]:

Looks good.

Review of attachment 309537 [details] [review]:

Seems like code motion that simplifies.

::: src/wayland/meta-wayland-pointer.c
@@ +863,3 @@
+  wl_list_insert_list (&surface->compositor->frame_callbacks,
+                       &pending->frame_callback_list);
+  wl_list_init (&pending->frame_callback_list);

But it's really not OK to have code that modifies fields of the surface structure scattered all over different files (probably this comment belongs earlier in this patchset).

Review of attachment 309535 [details] [review]:

Should have been done like this at the beginning - I'll comment on that on this role-gobjectification patch.

I don't see a value in private+getter instead of just putting this in the MetaWaylandSurfaceRole structure, but then again, we've made privates pretty efficient these days, so <shrug>, if you want to do the extra typing, it's fine. :-)

Review of attachment 309533 [details] [review]:

Hmm, this patch seems to fall between two things:

 A) An enumeration with methods - this is possible, for example, in Java, I could have a commit() method that I implement separately for each member of a role enumeration.
 B) A real aggregate object (decorator) that is assigned to the surface to make it gain new data and behaviors.

Of the two, I like B) better - because we actually have things like /* xdg_surface stuff */ in the MetaWaylandSurface type. As a long-term direction, I'd like to see data move to that auxiliary object. (Don't do that as part of this branch, however :-)

Is MetaWaylandSurfaceRole a good name for the baseclass? I've gone back and forth on my thinking on this - it's like having a Species class where the DogSpecies and CatSpecies subclasses have get_name() and get_age() methods - so certainly wrong at a nit-picky level. But on the other hand - I can't think that the code is going to get any easier to read if we have MetaWaylandSurfaceRoleDecorator and XdgSurfaceRoleDecorator rather than MetaWaylandSurfaceRole and XdgSurfaceRole - I don't think so. So let's go with this naming.

::: src/wayland/meta-wayland-surface.c
@@ +95,3 @@
+  if (!surface->role)
+    surface->role = g_object_new (role_type, NULL);
+  else if (G_OBJECT_TYPE (surface->role) != role_type)

Style: branches of an if statement be consistent - either {} or not.

@@ +208,3 @@
+  wl_list_insert_list (&surface->compositor->frame_callbacks,
+                       &pending->frame_callback_list);
+  wl_list_init (&pending->frame_callback_list);

This code definitely shouldn't be duplicated in multiple places ... it's hard enough to read in one place! Add a helper function.

@@ +287,3 @@
+  queue_surface_actor_frame_callbacks (surface, pending);
+
+  if (META_IS_WAYLAND_SURFACE_ROLE_WL_SHELL_SURFACE (surface->role))

Wouldn't it be better if the role type names were just MetaWlShellSurfaceRole, etc?

 META_IS_WL_SHELL_SURFACE_ROLE (surface->role)

@@ +2352,3 @@
+      wl_list_insert_list (&surface->frame_callback_list,
+                           &pending->frame_callback_list);
+      wl_list_init (&pending->frame_callback_list);

Normalize your GObject code, at the call site:

 if (surface->role)
    meta_wayland_surface_role_commit(surface->role, surface->pending);
 else 
    ...

@@ +2358,3 @@
+static gboolean
+meta_wayland_surface_role_is_on_output (MetaWaylandSurface *surface,
+                                        MetaMonitorInfo *monitor)

Same thing here: a function called - meta_wayland_surface_role_is_on_output() should never be called if the appropriate MetaWaylandSurfaceRole is null.

::: src/wayland/meta-wayland-surface.h
@@ +41,3 @@
                       GObject);
 
+typedef GType MetaWaylandSurfaceRoleType;

Please remove this typedef, it makes things more confusing.

@@ +52,3 @@
+
+  void (*commit) (MetaWaylandSurface      *surface,
+                  MetaWaylandPendingState *pending);

By not having the MetaWaylandSurfaceRole as the first member, you are creating a new "language feature" for GObject - don't do this. It makes the work of the person reading the code harder and also creates unbindable objects classes.

@@ +57,3 @@
+};
+
+#define META_DECLARE_WAYLAND_SURFACE_ROLE(ROLE_NAME, RoleName, role_name)     \

My advice - use your own judgment about whether you want to change the code or not, is that these macros aren't worth it and make it harder to understand the code.

Basically what you've done is replace 6 standard GObject definitions with 3 complex custom macros, plus code to use them - while you've reduced the total line count, you've introduced complexity "why is this different from all the other subclasses?" - oh, because there were 6 of them.

And when in a follow-up patch you need to open-code the definition of the cursor role to add a dispose() function, that further increases the complexity because META_DEFINE_WAYLAND_SURFACE_ROLE() can't even be considered to be opaque.

@@ +237,3 @@
+                                                  MetaWaylandSurfaceRoleType role_type,
+                                                  struct wl_resource        *error_resource,
+                                                  uint32_t                   error_code);

This is really not a setter - setter take a single argument that directly becomes a property of the object, and cannot fail. meta_surface_create_and_assign_role() would be a better name.

I wonder if this would be better open-coded in the setter:

 if (surface->role != NULL)
   {
     wl_resource_post_error (error_resource, error_code,
                             "wl_surface@%d already has a different role",
                             wl_resource_get_id (surface->resource));
     return;
   }

 meta_surface_set_role(surface, g_object_new(meta_dnd_surface_get_role(), NULL));

But while that is certainly clearer, and allows separating the check from the operation if necessary to cleanly not do anything on error, having to duplicate the error string many times across the code is a pain, so I'm OK with a renamed (and probably not-int-returning) version of this.

(In reply to Owen Taylor from comment #103)
> Review of attachment 309537 [details] [review] [review]:
> 
> Seems like code motion that simplifies.
> 
> ::: src/wayland/meta-wayland-pointer.c
> @@ +863,3 @@
> +  wl_list_insert_list (&surface->compositor->frame_callbacks,
> +                       &pending->frame_callback_list);
> +  wl_list_init (&pending->frame_callback_list);
> 
> But it's really not OK to have code that modifies fields of the surface
> structure scattered all over different files (probably this comment belongs
> earlier in this patchset).

This is not modifying the surface structure but the MetaWaylandPendingState. This is in a role-commit function, which purpose is to take what is in the pending state and apply it, so I think it needs to modify it in order to do that. Could use a helper though.

(In reply to Owen Taylor from comment #104)
> Review of attachment 309535 [details] [review] [review]:
> 
> Should have been done like this at the beginning - I'll comment on that on
> this role-gobjectification patch.
> 
> I don't see a value in private+getter instead of just putting this in the
> MetaWaylandSurfaceRole structure, but then again, we've made privates pretty
> efficient these days, so <shrug>, if you want to do the extra typing, it's
> fine. :-)

G_DECLARE_DERIVABLE_TYPE defines the MetaWaylandSuraceRole struct to be empty except for the parent, so we can't do that. It's also part of my secret plan to not have any struct definitions in the headers.

(In reply to Owen Taylor from comment #105)
> Review of attachment 309533 [details] [review] [review]:
> 
> Hmm, this patch seems to fall between two things:
> 
>  A) An enumeration with methods - this is possible, for example, in Java, I
> could have a commit() method that I implement separately for each member of
> a role enumeration.
>  B) A real aggregate object (decorator) that is assigned to the surface to
> make it gain new data and behaviors.

B) has been the whole purpose of this. Could you point out where I made things as A) describe? I know there are a couple of if (type == X) do this, but didn't want to make the patch too huge.

> 
> Of the two, I like B) better - because we actually have things like /*
> xdg_surface stuff */ in the MetaWaylandSurface type. As a long-term
> direction, I'd like to see data move to that auxiliary object. (Don't do
> that as part of this branch, however :-)

That has been a longer term goal with the role object.

> 
> Is MetaWaylandSurfaceRole a good name for the baseclass? I've gone back and
> forth on my thinking on this - it's like having a Species class where the
> DogSpecies and CatSpecies subclasses have get_name() and get_age() methods -
> so certainly wrong at a nit-picky level. But on the other hand - I can't
> think that the code is going to get any easier to read if we have
> MetaWaylandSurfaceRoleDecorator and XdgSurfaceRoleDecorator rather than
> MetaWaylandSurfaceRole and XdgSurfaceRole - I don't think so. So let's go
> with this naming.

Yea, lets go with just Role. Decorator can be easily confused with graphical decoration which we also do.

> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +95,3 @@
> +  if (!surface->role)
> +    surface->role = g_object_new (role_type, NULL);
> +  else if (G_OBJECT_TYPE (surface->role) != role_type)
> 
> Style: branches of an if statement be consistent - either {} or not.
> 
> @@ +208,3 @@
> +  wl_list_insert_list (&surface->compositor->frame_callbacks,
> +                       &pending->frame_callback_list);
> +  wl_list_init (&pending->frame_callback_list);
> 
> This code definitely shouldn't be duplicated in multiple places ... it's
> hard enough to read in one place! Add a helper function.
> 
> @@ +287,3 @@
> +  queue_surface_actor_frame_callbacks (surface, pending);
> +
> +  if (META_IS_WAYLAND_SURFACE_ROLE_WL_SHELL_SURFACE (surface->role))
> 
> Wouldn't it be better if the role type names were just
> MetaWlShellSurfaceRole, etc?

I suppose it would. I just followed the convention of putting Wayland things behind a MetaWayland. While its nice to be informative in the names, they are indeed a bit long. I'm torn here.

> 
>  META_IS_WL_SHELL_SURFACE_ROLE (surface->role)
> 
> @@ +2352,3 @@
> +      wl_list_insert_list (&surface->frame_callback_list,
> +                           &pending->frame_callback_list);
> +      wl_list_init (&pending->frame_callback_list);
> 
> Normalize your GObject code, at the call site:
> 
>  if (surface->role)
>     meta_wayland_surface_role_commit(surface->role, surface->pending);
>  else 
>     ...
> 
> @@ +2358,3 @@
> +static gboolean
> +meta_wayland_surface_role_is_on_output (MetaWaylandSurface *surface,
> +                                        MetaMonitorInfo *monitor)
> 
> Same thing here: a function called -
> meta_wayland_surface_role_is_on_output() should never be called if the
> appropriate MetaWaylandSurfaceRole is null.
> 
> ::: src/wayland/meta-wayland-surface.h
> @@ +41,3 @@
>                        GObject);
>  
> +typedef GType MetaWaylandSurfaceRoleType;
> 
> Please remove this typedef, it makes things more confusing.
> 
> @@ +52,3 @@
> +
> +  void (*commit) (MetaWaylandSurface      *surface,
> +                  MetaWaylandPendingState *pending);
> 
> By not having the MetaWaylandSurfaceRole as the first member, you are
> creating a new "language feature" for GObject - don't do this. It makes the
> work of the person reading the code harder and also creates unbindable
> objects classes.
> 
> @@ +57,3 @@
> +};
> +
> +#define META_DECLARE_WAYLAND_SURFACE_ROLE(ROLE_NAME, RoleName, role_name)  
> \
> 
> My advice - use your own judgment about whether you want to change the code
> or not, is that these macros aren't worth it and make it harder to
> understand the code.
> 
> Basically what you've done is replace 6 standard GObject definitions with 3
> complex custom macros, plus code to use them - while you've reduced the
> total line count, you've introduced complexity "why is this different from
> all the other subclasses?" - oh, because there were 6 of them.
> 
> And when in a follow-up patch you need to open-code the definition of the
> cursor role to add a dispose() function, that further increases the
> complexity because META_DEFINE_WAYLAND_SURFACE_ROLE() can't even be
> considered to be opaque.

Yea, I've been rethinking using macros. In the future more and more will not be using them anyway as well, so I'm fine with open coding it.

> 
> @@ +237,3 @@
> +                                                 
> MetaWaylandSurfaceRoleType role_type,
> +                                                  struct wl_resource       
> *error_resource,
> +                                                  uint32_t                 
> error_code);
> 
> This is really not a setter - setter take a single argument that directly
> becomes a property of the object, and cannot fail.
> meta_surface_create_and_assign_role() would be a better name.
> 
> I wonder if this would be better open-coded in the setter:
> 
>  if (surface->role != NULL)
>    {
>      wl_resource_post_error (error_resource, error_code,
>                              "wl_surface@%d already has a different role",
>                              wl_resource_get_id (surface->resource));
>      return;
>    }
> 
>  meta_surface_set_role(surface, g_object_new(meta_dnd_surface_get_role(),
> NULL));

Maybe even if (meta_wayland_surface_assign_role (surface, ..._get_type()) != 0) wl_resource_post_error (...); Because the role assigning check is not as simple as a NULL check.

> 
> But while that is certainly clearer, and allows separating the check from
> the operation if necessary to cleanly not do anything on error, having to
> duplicate the error string many times across the code is a pain, so I'm OK
> with a renamed (and probably not-int-returning) version of this.

What do you mean with not int returning? What should it return then? The caller still needs to know whether the assignment succeeded or not.

Attachment 309526 [details] pushed as 4b667d1 - Rename MetaCursorReference to MetaCursorSprite
Attachment 309527 [details] pushed as b01f95c - Make MetaCursorSprite a GObject
Attachment 309528 [details] pushed as cd1ce2c - MetaWaylandSurface: Make it a GObject
Attachment 309529 [details] pushed as 68279e8 - MetaWaylandPointer: Don't keep our own MetaCursorTracker pointer
Attachment 309530 [details] pushed as 165050f - backends: Get rid of meta-cursor-private.h
Attachment 309531 [details] pushed as d3fdaa3 - MetaCursorSprite: Put renderer specific code in the renderer

Review of attachment 309538 [details] [review]:

Didn't quite finish going through meta-wayland-pointer.[ch], but here is a first pass at some comments... I think the main one is that I'd prefer if the "buffer swapping" in meta-cursor-renderer-native.c was done in a more logical fashion - currently it seems to flip things around "randomly" and hope for the best.

::: src/backends/meta-cursor-renderer.c
@@ +65,2 @@
   if (priv->displayed_cursor && !priv->handled_by_backend)
+    texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor);

Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite passed in when it seems like it will always be priv->displayed_cursor?

@@ +91,3 @@
+MetaRectangle
+meta_cursor_renderer_calculate_rect (MetaCursorRenderer *renderer,
+                                     MetaCursorSprite *cursor_sprite)

In general we don't use struct returns in Mutter or elsewhere in GNOME - there's not really a big reason for that with modern ABIs... I'd probably prefer consistency, but don't care that much, so feel free to leave this.

@@ +111,3 @@
+  return (MetaRectangle) {
+    .x = (int)(priv->current_x - (hot_x * texture_scale)),
+    .y = (int)(priv->current_y - (hot_y * texture_scale)),

Seems rounding would be more appropriate

::: src/backends/meta-cursor-renderer.h
@@ +44,3 @@
 
+  gboolean (* update_cursor) (MetaCursorRenderer *renderer,
+                              MetaCursorSprite *cursor_sprite);

I found it confusing reading the code that:

 update_cursor

Updates the internal state of the *renderer* based on the cursor_sprite. While

 realize_cursor_for_wl_buffer

Updates the state of the *cursor*. 

I don't really understand why the change to add the cursor_sprite parameter was needed, and I think it contributes to the problem by making the non-parallel methods in the interface look more parallel.

::: src/backends/native/meta-cursor-renderer-native.c
@@ +62,3 @@
+{
+  guint current_bo;
+  struct gbm_bo *bos[2];

A comment about why the two bos would be *very* useful.

This was discussed some in IRC and email, and it turns out that the likely situation that was going wrong and causing glitches was:

 We call drmModeSetCursor2() with buffer A
 We call gbm_bo_destroy() on the old buffer
 Instead of freeing the old buffer, the intel libdrm driver stores it in a client-side pool.
 We call gbm_bo_create() and get the *same* BO back - which is still being scanned out as the cursor
 We modify it, causing the glitch

I think for now we can consider that drmModeSetCursor2() completely replaces any references to previously set cursors and at that point the old buffer can be reused, so that mostly the approach you are taking here should work. 

As was noted in your email, the logic here isn't really right for handling that ,because the "flip" is done when the cursor is changed, rather than at the time the drmModeSetCursor2() call is called. To me, this makes the code here extremely hard to understand ... it logically doesn't hang together. Honestly, I'd like to see that fixed before this is committed even if it mostly works now.

@@ +144,3 @@
+      bo = get_current_cursor_sprite_gbm_bo (cursor_sprite);
+      dirty = gbm_bo_get_user_data (bo);
+      if (!force && bo == crtc->cursor_renderer_private && !*dirty)

What's to prevent:

 Old cursor sprite is freed, including the bo's
 New cursor sprite is allocated with the *same address*

This may seem rather unlikely - and it is - but it could happen, and we should in general not be writing code that compares to potentially freed pointers.

====

I think if you switch when you call drmModeSetCursor2 you don't need the dirty flag business - you just need a flag in the cursor_sprite saying whether you have a pending new buffer you need to set.

@@ +210,3 @@
 static gboolean
+should_have_hw_cursor (MetaCursorRenderer *renderer,
+                       MetaCursorSprite *cursor_sprite)

Mutter is supposed to have GTK+-style annoying alignment of function parameters ;-)

@@ +222,3 @@
+
+  if (meta_cursor_sprite_get_texture_scale (cursor_sprite) != 1)
+    return FALSE;

Some day we can think about sticking cursor in planes on relevant hardware (that's what happens internally in the kernel for Intel hardware anyways...) and then we can get the hardware to scale cursors up/down when necessary.

::: src/core/boxes-private.h
@@ +41,3 @@
+gboolean meta_rectangle_contains_point  (const MetaRectangle *rect,
+                                         int                  x,
+                                         int                  y);

Seems to duplicate the POINT_IN_RECT() macro in common.h

Review of attachment 309538 [details] [review]:

I have this feeling that it somehow could be simpler (like why does realization have to be triggered throughout the code - couldn't it just be done by the backend when needed?) but as far as I can follow the code it seems like it should work. Some more style comments follow:

::: src/backends/meta-cursor.c
@@ +350,3 @@
   object_class->finalize = meta_cursor_sprite_finalize;
+
+  signals[PRE_PAINT_AT] = g_signal_new ("pre-paint-at",

I dislike this naming - the other places where we use "pre-paint", we use it specifically for things done in the clutter PRE_PAINT phase. "position-updated" would be better.

::: src/wayland/meta-wayland-pointer.c
@@ +809,3 @@
+  prev_cursor_surface = pointer->cursor_surface;
+  if (prev_cursor_surface)
+    g_signal_handlers_disconnect_by_data (cursor_tracker, prev_cursor_surface);

Neither cursor_tracker nor prev_cursor_surface is private to this part of the code, so you can't be *sure* that nobody else has made a connection. g_signal_handlers_disconnect_by_func() should always be used unless the data is truly private.

Is there any reason not to make the connection *once*, and simply update pointer->cursor_surface, rather than connecting like this?

@@ +848,3 @@
+        {
+          cursor_role->cursor_sprite = meta_cursor_sprite_new ();
+          g_signal_connect_object (cursor_role->cursor_sprite,

I'd honestly just disconnect in the dispose function for the cursor role - it's a lot more straightforward. With this, there is going to be some interval during destruction of the cursor sprite where the signal is disconnected but cursor_role->cursor sprite is set, or vice versa, and that can lead to head-banging hard bugs. Maybe not here, but I'd consider this a pattern to avoid

@@ +942,3 @@
 
+static void
+cursor_surface_dispose (GObject *object)

surface_role_cursor_dispose() [or meta_wayland_surface_role_cursor_dispose()] it should be clear what object it's a dispose function for.

Review of attachment 309540 [details] [review]:

Sure.

So, I just tried to track down the code path (in your branch) that would queue of a redraw from cursor_surface_commit() if a hardware cursor wasn't used, and didnt' find it. How does that work?

(In reply to Owen Taylor from comment #111)
> Review of attachment 309538 [details] [review] [review]:
> 
> I have this feeling that it somehow could be simpler (like why does
> realization have to be triggered throughout the code - couldn't it just be
> done by the backend when needed?) but as far as I can follow the code it
> seems like it should work. Some more style comments follow:

This is because the data backing the cursor may not part of the actual cursor (the wl_surface) so if we'd want to realize it just before drawing, we wouldn't know how to realize it, since the cursor doesn't know about the wl_surface. We could probably add a signal instead that the creator needs to set, or a "backing object" so that the renderer can call "cursor->backing->realize()" or something. With the current approach the renderer specific realization (which is what realize does) is done at creation when the source of the data is known.

> 
> ::: src/backends/meta-cursor.c
> @@ +350,3 @@
>    object_class->finalize = meta_cursor_sprite_finalize;
> +
> +  signals[PRE_PAINT_AT] = g_signal_new ("pre-paint-at",
> 
> I dislike this naming - the other places where we use "pre-paint", we use it
> specifically for things done in the clutter PRE_PAINT phase.
> "position-updated" would be better.

"position-updated" is not really correct though (I realize the calling function name makes it look like it), but it will be signalled even if the position didn't change, but the cursor changed. Maybe just "cursor-updated" or something.

> 
> ::: src/wayland/meta-wayland-pointer.c
> @@ +809,3 @@
> +  prev_cursor_surface = pointer->cursor_surface;
> +  if (prev_cursor_surface)
> +    g_signal_handlers_disconnect_by_data (cursor_tracker,
> prev_cursor_surface);
> 
> Neither cursor_tracker nor prev_cursor_surface is private to this part of
> the code, so you can't be *sure* that nobody else has made a connection.
> g_signal_handlers_disconnect_by_func() should always be used unless the data
> is truly private.

Well, we can't disconnect by_func because that would make life harder if we one day want to support multiple seats.

> 
> Is there any reason not to make the connection *once*, and simply update
> pointer->cursor_surface, rather than connecting like this?

I suppose that might work. And can just use a handler id to disconnect to avoid any trouble with disconnecting.

> 
> @@ +848,3 @@
> +        {
> +          cursor_role->cursor_sprite = meta_cursor_sprite_new ();
> +          g_signal_connect_object (cursor_role->cursor_sprite,
> 
> I'd honestly just disconnect in the dispose function for the cursor role -
> it's a lot more straightforward. With this, there is going to be some
> interval during destruction of the cursor sprite where the signal is
> disconnected but cursor_role->cursor sprite is set, or vice versa, and that
> can lead to head-banging hard bugs. Maybe not here, but I'd consider this a
> pattern to avoid

Indeed. I already do this locally, because there was an occasional crash where the listener assumed there was a valid surface->role pointer.

> 
> @@ +942,3 @@
>  
> +static void
> +cursor_surface_dispose (GObject *object)
> 
> surface_role_cursor_dispose() [or
> meta_wayland_surface_role_cursor_dispose()] it should be clear what object
> it's a dispose function for.

(In reply to Owen Taylor from comment #113)
> So, I just tried to track down the code path (in your branch) that would
> queue of a redraw from cursor_surface_commit() if a hardware cursor wasn't
> used, and didnt' find it. How does that work?

It doesn't seem to work. Didn't work previously either it seems. It needs to be fixed, but considering that its not a regression, I don't think it should be a blocker.

(In reply to Jonas Ådahl from comment #114)
> (In reply to Owen Taylor from comment #111)
> > Review of attachment 309538 [details] [review] [review] [review]:
> > 
> > I have this feeling that it somehow could be simpler (like why does
> > realization have to be triggered throughout the code - couldn't it just be
> > done by the backend when needed?) but as far as I can follow the code it
> > seems like it should work. Some more style comments follow:
> 
> This is because the data backing the cursor may not part of the actual
> cursor (the wl_surface) so if we'd want to realize it just before drawing,
> we wouldn't know how to realize it, since the cursor doesn't know about the
> wl_surface. We could probably add a signal instead that the creator needs to
> set, or a "backing object" so that the renderer can call
> "cursor->backing->realize()" or something. With the current approach the
> renderer specific realization (which is what realize does) is done at
> creation when the source of the data is known.

That's not really what "realize" means in a GTK+/Clutter context - realize means "create the resources now that everything is set up". It would seem to me there would be more flexibility from cursor_sprite_set_wl_buffer/xcursor that stored them, and let the cursor renderer pick up the data and create further backend resources at need. But anyways, let's get this landed.
 
> > ::: src/backends/meta-cursor.c
> > @@ +350,3 @@
> >    object_class->finalize = meta_cursor_sprite_finalize;
> > +
> > +  signals[PRE_PAINT_AT] = g_signal_new ("pre-paint-at",
> > 
> > I dislike this naming - the other places where we use "pre-paint", we use it
> > specifically for things done in the clutter PRE_PAINT phase.
> > "position-updated" would be better.
> 
> "position-updated" is not really correct though (I realize the calling
> function name makes it look like it), but it will be signalled even if the
> position didn't change, but the cursor changed. Maybe just "cursor-updated"
> or something.

What about "prepare-at" ?
 
> > ::: src/wayland/meta-wayland-pointer.c
> > @@ +809,3 @@
> > +  prev_cursor_surface = pointer->cursor_surface;
> > +  if (prev_cursor_surface)
> > +    g_signal_handlers_disconnect_by_data (cursor_tracker,
> > prev_cursor_surface);
> > 
> > Neither cursor_tracker nor prev_cursor_surface is private to this part of
> > the code, so you can't be *sure* that nobody else has made a connection.
> > g_signal_handlers_disconnect_by_func() should always be used unless the data
> > is truly private.
> 
> Well, we can't disconnect by_func because that would make life harder if we
> one day want to support multiple seats.

The naming doesn't make it obvious, but disconnect_by_func takes the function *and* data.

I have just pushed a new version of the wip/hidpi-cursor branch to github. Do you want me to update the patches in this bug?

In summary, it fixes most of the issues you raised, but some things I will comment on more explicitly:

I did not change nor rename any of the role-ness of XWayland wl_surfaces. I believe making it a role internally still makes most sense.

(In reply to Owen Taylor from comment #110)
> Review of attachment 309538 [details] [review] [review]:
> 
> ::: src/backends/meta-cursor-renderer.c
> @@ +65,2 @@
>    if (priv->displayed_cursor && !priv->handled_by_backend)
> +    texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor);
> 
> Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite
> passed in when it seems like it will always be priv->displayed_cursor?

I still avoid using renderer_priv->displayed_cursor or meta_cursor_renderer_get_cursor() because my longer-term intention is to get rid of that state, considering that it is just a duplication of the same state tracked by meta-cursor-tracker.c. I avoid it even further in the newer version, but not completely.

> ::: src/backends/meta-cursor-renderer.h
> @@ +44,3 @@
>  
> +  gboolean (* update_cursor) (MetaCursorRenderer *renderer,
> +                              MetaCursorSprite *cursor_sprite);
> 
> I found it confusing reading the code that:
> 
>  update_cursor
> 
> Updates the internal state of the *renderer* based on the cursor_sprite.
> While
> 
>  realize_cursor_for_wl_buffer
> 
> Updates the state of the *cursor*. 
> 
> I don't really understand why the change to add the cursor_sprite parameter
> was needed, and I think it contributes to the problem by making the
> non-parallel methods in the interface look more parallel.
> 

I pass the sprite because I don't want to add more uses of priv->displayed_cursor (explained above).

> ::: src/backends/native/meta-cursor-renderer-native.c
> @@ +62,3 @@
> +{
> +  guint current_bo;
> +  struct gbm_bo *bos[2];
> 
> A comment about why the two bos would be *very* useful.

I actually needed to use an array of three, along with an explanation in the relevant patch.

> 
> This was discussed some in IRC and email, and it turns out that the likely
> situation that was going wrong and causing glitches was:
> 
>  We call drmModeSetCursor2() with buffer A
>  We call gbm_bo_destroy() on the old buffer
>  Instead of freeing the old buffer, the intel libdrm driver stores it in a
> client-side pool.
>  We call gbm_bo_create() and get the *same* BO back - which is still being
> scanned out as the cursor
>  We modify it, causing the glitch
> 
> I think for now we can consider that drmModeSetCursor2() completely replaces
> any references to previously set cursors and at that point the old buffer
> can be reused, so that mostly the approach you are taking here should work. 
> 
> As was noted in your email, the logic here isn't really right for handling
> that ,because the "flip" is done when the cursor is changed, rather than at
> the time the drmModeSetCursor2() call is called. To me, this makes the code
> here extremely hard to understand ... it logically doesn't hang together.
> Honestly, I'd like to see that fixed before this is committed even if it
> mostly works now.

The new version of the big-chunk-patch does this now. It could even more refactoring, but in summary, drmModeSetCursor2() is now called once per frame only, and updating the cursor will queue a redraw which will cause calling of the whole stage to be redrawn (well, not really, because there may be no damage) with all the drm ioctls with it.

(In reply to Owen Taylor from comment #116)
> (In reply to Jonas Ådahl from comment #114)
> > (In reply to Owen Taylor from comment #111)
> > > Review of attachment 309538 [details] [review] [review] [review] [review]:
> > > 
> > > I have this feeling that it somehow could be simpler (like why does
> > > realization have to be triggered throughout the code - couldn't it just be
> > > done by the backend when needed?) but as far as I can follow the code it
> > > seems like it should work. Some more style comments follow:
> > 
> > This is because the data backing the cursor may not part of the actual
> > cursor (the wl_surface) so if we'd want to realize it just before drawing,
> > we wouldn't know how to realize it, since the cursor doesn't know about the
> > wl_surface. We could probably add a signal instead that the creator needs to
> > set, or a "backing object" so that the renderer can call
> > "cursor->backing->realize()" or something. With the current approach the
> > renderer specific realization (which is what realize does) is done at
> > creation when the source of the data is known.
> 
> That's not really what "realize" means in a GTK+/Clutter context - realize
> means "create the resources now that everything is set up". It would seem to
> me there would be more flexibility from cursor_sprite_set_wl_buffer/xcursor
> that stored them, and let the cursor renderer pick up the data and create
> further backend resources at need. But anyways, let's get this landed.
>  

That is kind of why I used that naming. It creates the renderer specific backing after all the available information is available. But in the future, I'd like to make this happen just before the drmModeSetCursor2 once per frame, and avoid creating any unnecessary buffers, but I believe that can wait.

(In reply to Jonas Ådahl from comment #117)
> I have just pushed a new version of the wip/hidpi-cursor branch to github.
> Do you want me to update the patches in this bug?

It would be helpful to me if you reattached the branch, and then went through and marked a-c-n anything that either a) I already marked a-c-n and you haven't substantially changed b) anything where I asked for simple changes, and you went ahead and made them and don't feel a need for re-review.
 
> In summary, it fixes most of the issues you raised, but some things I will
> comment on more explicitly:
> 
> I did not change nor rename any of the role-ness of XWayland wl_surfaces. I
> believe making it a role internally still makes most sense.

OK, since you and Jasper seem to be in agreement that this makes sense, I"ll withdraw my objection.

> (In reply to Owen Taylor from comment #110)
> > Review of attachment 309538 [details] [review] [review] [review]:
> > 
> > ::: src/backends/meta-cursor-renderer.c
> > @@ +65,2 @@
> >    if (priv->displayed_cursor && !priv->handled_by_backend)
> > +    texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor);
> > 
> > Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite
> > passed in when it seems like it will always be priv->displayed_cursor?
> 
> I still avoid using renderer_priv->displayed_cursor or
> meta_cursor_renderer_get_cursor() because my longer-term intention is to get
> rid of that state, considering that it is just a duplication of the same
> state tracked by meta-cursor-tracker.c. I avoid it even further in the newer
> version, but not completely.
>
> > ::: src/backends/meta-cursor-renderer.h
> > @@ +44,3 @@
> >  
> > +  gboolean (* update_cursor) (MetaCursorRenderer *renderer,
> > +                              MetaCursorSprite *cursor_sprite);
> > 
> > I found it confusing reading the code that:
> > 
> >  update_cursor
> > 
> > Updates the internal state of the *renderer* based on the cursor_sprite.
> > While
> > 
> >  realize_cursor_for_wl_buffer
> > 
> > Updates the state of the *cursor*. 
> > 
> > I don't really understand why the change to add the cursor_sprite parameter
> > was needed, and I think it contributes to the problem by making the
> > non-parallel methods in the interface look more parallel.
> > 
> 
> I pass the sprite because I don't want to add more uses of
> priv->displayed_cursor (explained above).

If the tracker is considered to be the canonical place to keep track of the identity and position of the current MetaCursorSprite, it would make more sense for the renderer to *pull* the information it needs from the tracker, rather than to continually pass everything you want in - I can't imagine that you want:

void meta_cursor_renderer_set_position (MetaCursorRenderer *renderer,
                                        MetaCursorSprite *sprite,
                                        int x, int y);
void meta_cursor_renderer_set_cursor   (MetaCursorRenderer *renderer,
                                        MetaCursorSprite *sprite,
                                        int x, int y);

So adding this parameter seems to me to be going in the wrong direction. But let's try to get the branch landed - I don't think keeping it in-flight longer is doing us any good.

[...] 
 
> The new version of the big-chunk-patch does this now. It could even more 
> refactoring, but in summary, drmModeSetCursor2() is now called once per frame
> only, and updating the cursor will queue a redraw which will cause calling of 
> the whole stage to be redrawn (well, not really, because there may be no
> damage) with all the drm ioctls with it.

Have you tested in detail what happens when only moving the HW cursor? Do we *actually* not paint anything? Do we still page flip? 

> (In reply to Owen Taylor from comment #116)
> > (In reply to Jonas Ådahl from comment #114)
> > > (In reply to Owen Taylor from comment #111)
> > > > Review of attachment 309538 [details] [review] [review] [review] [review] [review]:
> > > > 
> > > > I have this feeling that it somehow could be simpler (like why does
> > > > realization have to be triggered throughout the code - couldn't it just be
> > > > done by the backend when needed?) but as far as I can follow the code it
> > > > seems like it should work. Some more style comments follow:
> > > 
> > > This is because the data backing the cursor may not part of the actual
> > > cursor (the wl_surface) so if we'd want to realize it just before drawing,
> > > we wouldn't know how to realize it, since the cursor doesn't know about the
> > > wl_surface. We could probably add a signal instead that the creator needs to
> > > set, or a "backing object" so that the renderer can call
> > > "cursor->backing->realize()" or something. With the current approach the
> > > renderer specific realization (which is what realize does) is done at
> > > creation when the source of the data is known.
> > 
> > That's not really what "realize" means in a GTK+/Clutter context - realize
> > means "create the resources now that everything is set up". It would seem to
> > me there would be more flexibility from cursor_sprite_set_wl_buffer/xcursor
> > that stored them, and let the cursor renderer pick up the data and create
> > further backend resources at need. But anyways, let's get this landed.
> >  
> 
> That is kind of why I used that naming. It creates the renderer specific
> backing after all the available information is available. But in the future,
> I'd like to make this happen just before the drmModeSetCursor2 once per
> frame, and avoid creating any unnecessary buffers, but I believe that can
> wait.

OK. I think things will be cleaned up long-term if we can go in the direction of making the renderer pull necessary information rather than continually poking down to the renderer layer.

Created attachment 310989 [details] [review]
wayland: Introduce XWayland surface role

Being a "XWayland window" should be considered equivalent to a role,
even though it is not part of any protocol anywhere. The commit doesn't
have any functional difference, but just makes it clear that an
wl_surface managed by XWayland have the same type of special casing as
surface roles as defined by the Wayland protocol.

As the semantics are more explicit given the role is defined, a comment
explaining why the semantics need to be how they are was added.

Created attachment 310990 [details] [review]
MetaWaylandSurface: Don't respond to frame callback when role unassigned

If a surface doesn't have a role, the compositor will not know how, if
or when it will be painted. By adding it to the compositor frame
callback list, the compositor will respond to the client that the
surface has been drawn already which might not be true.

Instead, queue the frame callback in a list that is then processed when
the surface gets a role assigned. The compositor may then, given the
role the surface got, queue the frame callback accordingly.

Created attachment 310991 [details] [review]
wayland: GObject:ify surface roles

Make a surface roles into objects with vfuncs for things where there
before was a big switch statement. The declaration and definition
boilerplate is hidden behind C macros.

Created attachment 310992 [details] [review]
backends/x11: Draw our own cursor sprite when running nested

Use a specialized cursor renderer when running as a nested Wayand
compositor. This new renderer sets an empty X11 cursor and draws the
cursor as part of the stage using the generic cursor renderer drawing
path.

Created attachment 310993 [details] [review]
MetaCursorSprite: Squash MetaCurorImage into MetaCursorSprite

It fills little purpose on separating into a MetaCursorImage struct, so
lets squash in the three fields into the MetaCursorSprite object.

Created attachment 310994 [details] [review]
wayland: Move cursor surface role to meta-wayland-pointer.c

The wl_pointer assigns a role to a wl_surface, so it makes sense to put
the related logic there.

Created attachment 310995 [details] [review]
Support scaling of cursor sprites given what output they are on

This commits refactors cursor handling code and plugs in logic so that
cursor sprites changes appearance as it moves across the screen.
Renderers are adapted to handle the necessary functionality.

The logic for changing the cursor sprite appearance is done outside of
MetaCursorSprite, and actually where depends on what type of cursor it
is. In mutter we now have two types of cursors that may have their
appearance changed:

 - Themed cursors (aka root cursors)
 - wl_surface cursors

Themed cursors are created by MetaScreen and when created, when
applicable(*), it will extend the cursor via connecting to a signal
which is emitted everytime the cursor is moved. The signal handler will
calculate the expected scale given the monitor it is on and reload the
theme in a correct size when needed.

wl_surface cursors are created when a wl_surface is assigned the
"cursor" role, i.e. when a client calls wl_pointer.set_cursor. A
cursor role object is created which is connected to the cursor object
by the position signal, and will set a correct texture scale given what
monitor the cursor is on and what scale the wl_surface's active buffer
is in. It will also push new buffers to the same to the cursor object
when new ones are committed to the surface.

This commit also makes texture loading lazy, since the renderer doesn't
calculate a rectangle when the cursor position changes.

The native backend is refactored to be triple-buffered; see the comment
in meta-cursor-renderer-native.c for further explanations.

* when we are running as a Wayland compositor

Created attachment 310996 [details] [review]
wayland: Support sending wl_surface.enter/leave to cursor surfaces

Support notifying clients about what outputs their cursor surfaces are
on so that they can attach appropriately scaled buffers to them.

Created attachment 310998 [details] [review]
MetaCursorRenderer: Rely on update_cursor for redrawing

Calling queue_redraw() in _force_update() is not needed because
update_cursor() will do this when needed, i.e. when switching between
hardware cursor and texture cursor, or when drawing with texture cursor.

There is also no need to force _native_force_update() because
update_cursor() will cover this as well when needed. When not changing
cursor but only the gbm_bo, the "dirty" boolean on the gbm_bo will
trigger a redraw.

Review of attachment 310992 [details] [review]:

Was previously accepted.

Review of attachment 310993 [details] [review]:

Was previously accepted.

Review of attachment 310998 [details] [review]:

Was previously accepted.

(In reply to Owen Taylor from comment #118)
> (In reply to Jonas Ådahl from comment #117) 
>  
> > The new version of the big-chunk-patch does this now. It could even more 
> > refactoring, but in summary, drmModeSetCursor2() is now called once per frame
> > only, and updating the cursor will queue a redraw which will cause calling of 
> > the whole stage to be redrawn (well, not really, because there may be no
> > damage) with all the drm ioctls with it.
> 
> Have you tested in detail what happens when only moving the HW cursor? Do we
> *actually* not paint anything? Do we still page flip? 

The side effect of doing it this way was that I needed to do a 1x1 pixel damage. So given this, as per IRC discussion, lets wait with doing things like this until we have better control over KMS interaction and can use atomic modesetting. The attached version calls the ioctls in the same way as before, with only the gbm_bo managing changed.

Anyhow, the new patches are attached, and I went through the ones that were previously accepted and marked them as such.

Review of attachment 310996 [details] [review]:

One vfunc needs a rename to be consistent, otherwise good to commit

::: src/wayland/meta-wayland-pointer.c
@@ +806,3 @@
 
+  if (cursor_sprite == meta_cursor_tracker_get_displayed_cursor (cursor_tracker))
+    meta_cursor_renderer_force_update (cursor_renderer);

Don't see what this has to do with this commit, but OK

@@ +982,3 @@
 
+static gboolean
+is_cursor_surface_role_on_monitor (MetaWaylandSurfaceRole *role,

Don't make up arbitrary names here - this should be:

 surface_role_is_on_output

@@ +993,3 @@
+  MetaWaylandSurfaceRoleCursor *cursor_role =
+    META_WAYLAND_SURFACE_ROLE_CURSOR (surface->role);
+  MetaCursorSprite *displayed_cursor_sprite;

No change needed, but I wanted to mention that there is no line length limit in Mutter source code, and the goal of introducing line breaks is to maximize readability.

Review of attachment 310989 [details] [review]:

I agree that this commit doesn't change behavior. The comment however, makes no sense to me - when I try to work out in detail why putting Xwayland windows through this code path prevens initial black flashes, I fail.

::: src/wayland/meta-wayland-surface.c
@@ +551,3 @@
+       * complying with the frame callback specification is that XWayland will not
+       * post any damage until after we map the surface actor, and we would
+       * initially draw the inital content (usually black).

So, let's consider the case of an undecorated window - which is the simple case that in classic X apps can get right without any flashing. The app does, for example:

 Set window background to none 
 MapWindow
 draw to an offscreen pixmap
 CopyArea to the window

This doesn't flash in the X composited case  because of "backfilling" - the newly allocated pixmap backing the window is filled in with a copy of that area of the root window, so it's "transparent". (The window's shadow might show up before it's contents, however.)  

[ Note: GTK+ 3 with the extended frame sync protocol and mutter gets this entirely right and waits for the app to be done before showing the window, instead of counting on backfilling - but we can assume that GTK+ 3 apps will be native Wayland ]

This sequence is inevitably prone to flashing on Xwayland because we can't do the backfilling and there is nothing in the X protocol to know when the app is "finished drawing"

So how does it matter whether we send frame callbacks:

 - After the window is drawn
 - After the next frame after we receive the commit with the damage is drawn

I can't see how it would, since we *only* can receive these commits after the X server has attached a buffer - and at this point we should already have mapped and be drawing the window.

Review of attachment 310990 [details] [review]:

Looks fine

Review of attachment 310994 [details] [review]:

OK

Review of attachment 310995 [details] [review]:

It looks like in this version, you are no longer reusing buffers, but always allocating new buffers. I wanted to note (perhaps for a future improvement) that if buffers are not being reused, then this can be simplified considerably, and the handling of avoiding messing with buffers that are being scanned out could be handled in done in the *renderer* rather than in the sprite, with a single BO attached to the sprite. update_hw_cursor then does:

 if (cursor_private->bo != renderer->current_bo) 
   {
      bo *last_bo = renderer->last_bo;
      renderer->last_bo = renderer->current_bo;

      renderer->current_bo = cursor_private->bo;
      drmModeSetCursor2(renderer->current_bo);

      if (last_bo)
        unref_bo(last_bo);
   }

The "bo" here is a trival structure that wraps struct gbm_bo with a reference count.

This approach would also fix the problem you have here (though it's not an important one) that if a cursor surface is *destroyed* by a client, it's buffers can no longer be kept referenced to prevent glitches.

Anyways, generally this looks like it should work, and I'm OK landing this as is, with a few minor fixes noted below.

::: src/backends/meta-cursor-renderer.h
@@ +59,3 @@
 MetaCursorRenderer * meta_cursor_renderer_new (void);
 
+void meta_cursor_renderer_pre_paint (MetaCursorRenderer *renderer);

Left-over

::: src/backends/native/meta-cursor-renderer-native.c
@@ +508,3 @@
+   * should unset, or succeed, which will set new buffer.
+   */
+  destroy_pending_cursor_sprite_gbm_bo (cursor_sprite);

Putting the private into an "invalid state", then having a later call to fix it up seems fiddly and fragile - why isn't the destroy part of the set_pending_cursor_sprite_gbm_bo[_failed]()? 

You could possibly just call set_pending_cursor_sprite_gbm_bo_failed() up front rather than in each failure path - similar to how you set the state to FAILED at initialization.

::: src/backends/x11/meta-cursor-renderer-x11.c
@@ +30,3 @@
 
+#include <meta/util.h>
+

Don't think this addition was needed

::: src/compositor/compositor.c
@@ +77,3 @@
 #include "meta-sync-ring.h"
 
+#include "backends/meta-stage.h"

extraneous?

@@ +1098,3 @@
         compositor->have_x11_sync_object = meta_sync_ring_insert_wait ();
       else
+        XSync (display->xdisplay, False);

all changes in this file seem to be leftoverse

(In reply to Owen Taylor from comment #133)
> Review of attachment 310989 [details] [review] [review]:
> 
> I agree that this commit doesn't change behavior. The comment however, makes
> no sense to me - when I try to work out in detail why putting Xwayland
> windows through this code path prevens initial black flashes, I fail.
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +551,3 @@
> +       * complying with the frame callback specification is that XWayland
> will not
> +       * post any damage until after we map the surface actor, and we would
> +       * initially draw the inital content (usually black).
> 
> So, let's consider the case of an undecorated window - which is the simple
> case that in classic X apps can get right without any flashing. The app
> does, for example:
> 
>  Set window background to none 
>  MapWindow
>  draw to an offscreen pixmap
>  CopyArea to the window
> 
> This doesn't flash in the X composited case  because of "backfilling" - the
> newly allocated pixmap backing the window is filled in with a copy of that
> area of the root window, so it's "transparent". (The window's shadow might
> show up before it's contents, however.)  
> 
> [ Note: GTK+ 3 with the extended frame sync protocol and mutter gets this
> entirely right and waits for the app to be done before showing the window,
> instead of counting on backfilling - but we can assume that GTK+ 3 apps will
> be native Wayland ]
> 
> This sequence is inevitably prone to flashing on Xwayland because we can't
> do the backfilling and there is nothing in the X protocol to know when the
> app is "finished drawing"
> 
> So how does it matter whether we send frame callbacks:
> 
>  - After the window is drawn
>  - After the next frame after we receive the commit with the damage is drawn
> 
> I can't see how it would, since we *only* can receive these commits after
> the X server has attached a buffer - and at this point we should already
> have mapped and be drawing the window.

The initial few buffers (haven't counted) XWayland attaches are black (maybe not GTK+ 3 X11 windows, they were not part of my test cases). Since XWayland throttles buffer attaching using wl_surface_frame (it wont attach a new buffer before receiving a frame callback), by waiting with replying with initial frame callbacks until we have mapped, we will effectively stop XWayland from attaching non-black buffers until we have already mapped and drawn the window already.

The non-change is to continue replying to wl_surface_frame even though the surface is not drawn so that XWayland can continue updating the content (to actual content) before we actually start to draw the window.

(In reply to Owen Taylor from comment #132)
> Review of attachment 310996 [details] [review] [review]:
> 
> @@ +993,3 @@
> +  MetaWaylandSurfaceRoleCursor *cursor_role =
> +    META_WAYLAND_SURFACE_ROLE_CURSOR (surface->role);
> +  MetaCursorSprite *displayed_cursor_sprite;
> 
> No change needed, but I wanted to mention that there is no line length limit
> in Mutter source code, and the goal of introducing line breaks is to
> maximize readability.

FWIW I more or less always use vertical split editor buffers, so long lines are usually unreadable in my setup.

In reply to Owen Taylor from comment #136)
> Review of attachment 310995 [details] [review] [review]:
> 
> It looks like in this version, you are no longer reusing buffers, but always
> allocating new buffers. I wanted to note (perhaps for a future improvement)
> that if buffers are not being reused, then this can be simplified
> considerably, and the handling of avoiding messing with buffers that are
> being scanned out could be handled in done in the *renderer* rather than in
> the sprite, with a single BO attached to the sprite.

Yea, I dropped that part, but didn't do any more major changes because its not the right time to redesign things just right now. I agree with you here anyhow.

> ::: src/backends/native/meta-cursor-renderer-native.c
> @@ +508,3 @@
> +   * should unset, or succeed, which will set new buffer.
> +   */
> +  destroy_pending_cursor_sprite_gbm_bo (cursor_sprite);
> 
> Putting the private into an "invalid state", then having a later call to fix
> it up seems fiddly and fragile - why isn't the destroy part of the
> set_pending_cursor_sprite_gbm_bo[_failed]()? 
> 
> You could possibly just call set_pending_cursor_sprite_gbm_bo_failed() up
> front rather than in each failure path - similar to how you set the state to
> FAILED at initialization.

Thought about doing it up front, but "FAILED" didn't make sense as a intermediate state. INVALIDATED would maybe make more sense. I'll go with that.

(In reply to Jonas Ådahl from comment #138)
> (In reply to Owen Taylor from comment #132)
> > Review of attachment 310996 [details] [review] [review] [review]:
> > 
> > @@ +993,3 @@
> > +  MetaWaylandSurfaceRoleCursor *cursor_role =
> > +    META_WAYLAND_SURFACE_ROLE_CURSOR (surface->role);
> > +  MetaCursorSprite *displayed_cursor_sprite;
> > 
> > No change needed, but I wanted to mention that there is no line length limit
> > in Mutter source code, and the goal of introducing line breaks is to
> > maximize readability.
> 
> FWIW I more or less always use vertical split editor buffers, so long lines
> are usually unreadable in my setup.

If you are working in a module where the normal line length practices are unreadable with your editor settings, then I think there is utility (perhaps even obligation) to adjust your editor settings for the duration of working on that module :-)

(In reply to Owen Taylor from comment #139)
> (In reply to Jonas Ådahl from comment #138)
> > (In reply to Owen Taylor from comment #132)
> > > Review of attachment 310996 [details] [review] [review] [review] [review]:
> > > 
> > > @@ +993,3 @@
> > > +  MetaWaylandSurfaceRoleCursor *cursor_role =
> > > +    META_WAYLAND_SURFACE_ROLE_CURSOR (surface->role);
> > > +  MetaCursorSprite *displayed_cursor_sprite;
> > > 
> > > No change needed, but I wanted to mention that there is no line length limit
> > > in Mutter source code, and the goal of introducing line breaks is to
> > > maximize readability.
> > 
> > FWIW I more or less always use vertical split editor buffers, so long lines
> > are usually unreadable in my setup.
> 
> If you are working in a module where the normal line length practices are
> unreadable with your editor settings, then I think there is utility (perhaps
> even obligation) to adjust your editor settings for the duration of working
> on that module :-)

Well, for mutter it is for the most case within a reasonable length. Its the 100+ long ones that look bad, and they are not that many.

(In reply to Jonas Ådahl from comment #137)

> Since XWayland
> throttles buffer attaching using wl_surface_frame (it wont attach a new
> buffer before receiving a frame callback), by waiting with replying with
> initial frame callbacks until we have mapped, we will effectively stop
> XWayland from attaching non-black buffers until we have already mapped and
> drawn the window already.

If the window isn't mapped, then there's no buffer attached, and we won't get a frame callback request from Xwayland - right?

(In reply to Owen Taylor from comment #141)
> (In reply to Jonas Ådahl from comment #137)
> 
> > Since XWayland
> > throttles buffer attaching using wl_surface_frame (it wont attach a new
> > buffer before receiving a frame callback), by waiting with replying with
> > initial frame callbacks until we have mapped, we will effectively stop
> > XWayland from attaching non-black buffers until we have already mapped and
> > drawn the window already.
> 
> If the window isn't mapped, then there's no buffer attached, and we won't
> get a frame callback request from Xwayland - right?

Mapped by mutter as in placed in the scene graph (yes, this term is used in Wayland). There may be buffers attached before mutter maps it, yes. XWayland has no idea whether mutter mapped its wl_surface or not.

Review of attachment 309854 [details] [review]:

::: src/wayland/meta-wayland-surface.c
@@ +566,3 @@
+       *  - Go through the surface actors frame callback list until some time after
+       *    it has been mapped so can avoid wasting buffers when the window is
+       *    hidden.

I'd like for us to say say:

 /* For Xwayland windows, throttling frames when the window isn't
  * actually drawn is less useful, because Xwayland still has to do
  * the drawing sent from the application - the throttling would only
  * be of sending us damage messages, so we simplify and send frame callbacks
  * after the next paint of the screen, whether the window was drawn
  * or not.
  * 
  * Currently in *some* cases we also seem to take a few frames before
  * we draw the window, for not completely understood reasons, and in
  * that case, not thottling frame callbacks to drawing has the
  * happy side effect that we avoid showing the user the initial black
  * frame from when the window is mapped empty.
  */

I can't find anything the code that intentionally tries to only show Xwayland windows on the second frame, and when I add printfs, I don't see this behavior - for me the first commit seems to be drawn with an already shown window. Reading the code looks like to me that Xwayland windows are shown only based on X traffic, without regard to wayland.

I suppose there perhaps is some sequence of, depending on the ordering of:

 receiving the WL_SURFACE_ID message 
 receiving the MapRequest/MapNotify message
 the commit messages

where things occur as you've noted, but I can't work out what is, and it definitely doesn't seem intentional or reliable.

Review of attachment 310991 [details] [review]:

Looks fine to, other than the propagation of the controversial Xwayland comment

Review of attachment 310991 [details] [review]:

Looks fine to, other than the propagation of the controversial Xwayland comment

(In reply to Owen Taylor from comment #143)
> Review of attachment 309854 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +566,3 @@
> +       *  - Go through the surface actors frame callback list until some
> time after
> +       *    it has been mapped so can avoid wasting buffers when the window
> is
> +       *    hidden.
> 
> I'd like for us to say say:
> 
>  /* For Xwayland windows, throttling frames when the window isn't
>   * actually drawn is less useful, because Xwayland still has to do
>   * the drawing sent from the application - the throttling would only
>   * be of sending us damage messages, so we simplify and send frame callbacks
>   * after the next paint of the screen, whether the window was drawn
>   * or not.
>   * 
>   * Currently in *some* cases we also seem to take a few frames before
>   * we draw the window, for not completely understood reasons, and in
>   * that case, not thottling frame callbacks to drawing has the
>   * happy side effect that we avoid showing the user the initial black
>   * frame from when the window is mapped empty.
>   */

"Some cases" is a bit misleading. From my testing, the only X clients that we seem to draw immediately after the role is assigned are GTK+ 3 clients, and this seems to be because GTK+ 3 implies MapNotify and creates the wl_surface a while after MapRequest while xterm, gitk, QT, GTK+ 2.0 all do MapRequest/MapNotify wl_surface creation up front.

> 
> I can't find anything the code that intentionally tries to only show
> Xwayland windows on the second frame, and when I add printfs, I don't see
> this behavior - for me the first commit seems to be drawn with an already
> shown window. Reading the code looks like to me that Xwayland windows are
> shown only based on X traffic, without regard to wayland.

I looked a bit more, and we do show XWayland MetaWindows up front when receiving MapRequest/MapNotify, sometimes long before it is possible to draw it (before we assigned a surface to it).

A difference between GTK+ 3 X11 clients and other X11 clients is that for GTK+ 3 clients, Xwayland creates the wl_surface a while after we receive MapRequest (i.e. create and show the MetaWindow). So, an observation I did was that no matter the client, the content tend to be drawn the third frame after we create (and show) the MetaWindow. For GTK+ 3 clients, this may be the the frame after the window<->wl_surface association is done. For other clients, its looks like it takes a frame or two more for it to be drawn, but its still seem to be usually three frames after the MetaWindow creation. I don't know what is causing the extra frames between the surface<->window association and the actual painting though.

> 
> I suppose there perhaps is some sequence of, depending on the ordering of:
> 
>  receiving the WL_SURFACE_ID message 
>  receiving the MapRequest/MapNotify message
>  the commit messages
> 
> where things occur as you've noted, but I can't work out what is, and it
> definitely doesn't seem intentional or reliable.

Attachment 310989 [details] pushed as dece49b - wayland: Introduce XWayland surface role
Attachment 310990 [details] pushed as 8e5fb03 - MetaWaylandSurface: Don't respond to frame callback when role unassigned
Attachment 310991 [details] pushed as 83c1713 - wayland: GObject:ify surface roles
Attachment 310992 [details] pushed as 8900bd2 - backends/x11: Draw our own cursor sprite when running nested
Attachment 310993 [details] pushed as e407f5b - MetaCursorSprite: Squash MetaCurorImage into MetaCursorSprite
Attachment 310994 [details] pushed as 7c7cf91 - wayland: Move cursor surface role to meta-wayland-pointer.c
Attachment 310995 [details] pushed as 79c86ae - Support scaling of cursor sprites given what output they are on
Attachment 310996 [details] pushed as 5d837a5 - wayland: Support sending wl_surface.enter/leave to cursor surfaces
Attachment 310998 [details] pushed as 0373b85 - MetaCursorRenderer: Rely on update_cursor for redrawing

Ever since these patches landed, I've got the correct size pointer in gtk apps, however now the pointer in gnome shell is HUGE, were before it was the correct size but tiny in gtk apps :-/

So should I reopen this bug, or file a new one?

(In reply to Bjørn Lie from comment #148)
> Ever since these patches landed, I've got the correct size pointer in gtk
> apps, however now the pointer in gnome shell is HUGE, were before it was the
> correct size but tiny in gtk apps :-/
> 
> So should I reopen this bug, or file a new one?

What you are experiencing is tracked in bug 755099.