Created attachment 300640 [details] [review]
nested: Handle pointer visibility on enter and leave events

When the pointer leaves the nested compositor stage window, always show
the server cursor so that if we were drawing it ourself (for example
when using a wl_surface or when emulating HiDPI cursor sprites) don't
end up with no cursor.

When the pointer enters the nested compositor stage window, always cause
the cursor renderer to redraw its cursor.

Created attachment 300641 [details] [review]
nested: Allow configuration of dummy output configuration

Review of attachment 300640 [details] [review]:

::: src/backends/x11/meta-backend-x11.c
@@ +144,3 @@
+      enter_event->mode == XINotifyUngrab) &&
+      (!meta_is_wayland_compositor () ||
+       enter_event->event != stage_window))

No change necessary, but I think it would read clearer as:

 !(meta_is_wayland_compositor () && enter_event->event == stage_window))

::: src/backends/x11/meta-cursor-renderer-x11.c
@@ +140,3 @@
+  XFixesShowCursor (xdisplay, xwindow);
+  priv->server_cursor_visible = TRUE;
+  meta_stage_set_cursor (META_STAGE (stage), NULL, &(MetaRectangle){ 0 });

You are really going behind the back of MetaCursorRenderer here. Just because you are adding logic that is only needed in the nested case doesn't mean you should add API to MetaCursorRendererX11 - what you are trying to do would be much better expressed as an API such as meta_cursor_renderer_set_cursor_hidden().

For an example of why what you've done here is unrobust, if meta_cursor_renderer_set_cursor() is called while the pointer is not in the window, the cursor will get shown again.

[ I note that Using XFixesHideCursor/XFixesShowCursor is actually not necessary because X cursors are inherently not displayed when the cursor is not in the window. It's OK if Hide/ShowCursor happens implicitly because of the implementation of set_cursor_hidden(), but it's weird to be explicitly doing it here. ]

Also, keep in mind that XFixesHideCursor/ShowCursor does not follow its documentation. It will hide/show the cursor *globally*, rather than on just the passed in window. To hide/show the cursor on just a window, use XDefineCursor to establish an empty cursor. We should probably do that instead.

Review of attachment 300641 [details] [review]:

Generally seems fine to me. A few small comments.

::: src/backends/meta-monitor-manager-dummy.c
@@ +59,3 @@
+   * environmental variables that can be used:
+   *
+   * MUTTER_DEBUG_NUM_DUMMY_OUTPUTS

There's definitely inconsistency here:

 MUTTER_DUMMY_MONITORS
 META_DEBUG_NUM_DUMMY_OUTPUTS

I tend to think that DEBUG in the context of mutter really refers to debug logging - MUTTER_DEBUG=1.

I'd suggest using MONITORS not OUTPUTS in the environment variable names, since to the extent they are different:

 outputs are what are configured in the hardware 
 monitors are what the user sees

With the difference being 4k monitors that appear as two monitors at the video level.

@@ +94,3 @@
+      gchar **scales_str_list;
+
+      scales_str_list = g_strsplit (output_scales_str, ",", num_outputs);

Not worth spending much time on debug code, but I think this code is rather obscure in that if you pass in:
 
 NUM_DUMMY_OUTPUTS=1
 DUMMY_OUTPUT_SCALES=1,2

that silently succeeds because strsplit() splits the list into ["1,2"] and then strtoll() ignores the trailing ,2. I'd split without a limit because the split-with-a-limit behavior is never what anybody wants, then do some explicit:

 num_scales = MIN(g_strlen(scales_str_list), num_outputs);

(or warn if num_scales does not much num_outputs)

(In reply to Jasper St. Pierre from comment #4)
> Also, keep in mind that XFixesHideCursor/ShowCursor does not follow its
> documentation. It will hide/show the cursor *globally*, rather than on just
> the passed in window. To hide/show the cursor on just a window, use
> XDefineCursor to establish an empty cursor. We should probably do that
> instead.

Should we do that in just the nested case or how would that work if we're a CM?

(In reply to Owen Taylor from comment #3)
> Review of attachment 300640 [details] [review] [review]:
> 
> ::: src/backends/x11/meta-backend-x11.c
> @@ +144,3 @@
> +      enter_event->mode == XINotifyUngrab) &&
> +      (!meta_is_wayland_compositor () ||
> +       enter_event->event != stage_window))
> 
> No change necessary, but I think it would read clearer as:
> 
>  !(meta_is_wayland_compositor () && enter_event->event == stage_window))
> 
> ::: src/backends/x11/meta-cursor-renderer-x11.c
> @@ +140,3 @@
> +  XFixesShowCursor (xdisplay, xwindow);
> +  priv->server_cursor_visible = TRUE;
> +  meta_stage_set_cursor (META_STAGE (stage), NULL, &(MetaRectangle){ 0 });
> 
> You are really going behind the back of MetaCursorRenderer here. Just
> because you are adding logic that is only needed in the nested case doesn't
> mean you should add API to MetaCursorRendererX11 - what you are trying to do
> would be much better expressed as an API such as
> meta_cursor_renderer_set_cursor_hidden().

I suppose that could work as well, but it makes it sound like an API that is supposed to be used hide the cursor. meta_cursor_renderer_set_cursor_hidden(true) would actually just show the cursor on the X server, while hiding it internally, and _set_cursor_hidden(false) would maybe hide the X server cursor, and maybe show the internal cursor. Not sure that API is more any better.

Could move it to MetaCursorRenderer (keeping some left/enterd naming) and then make it a no-op in the native backend.

> 
> For an example of why what you've done here is unrobust, if
> meta_cursor_renderer_set_cursor() is called while the pointer is not in the
> window, the cursor will get shown again.

It wouldn't since no client would (should at least) have pointer focus. Except if mutter/gnome-shell did it itself. Not sure it matters much, since this is just meant to make debugging mutter in nested mode more pleasent.

> 
> [ I note that Using XFixesHideCursor/XFixesShowCursor is actually not
> necessary because X cursors are inherently not displayed when the cursor is
> not in the window. It's OK if Hide/ShowCursor happens implicitly because of
> the implementation of set_cursor_hidden(), but it's weird to be explicitly
> doing it here. ]

The point of this patch was really just to call XFixesHideCursor/XFixesShowCursor because otherwise leaving the nested mutter window where the last position in mutter was over a surface (i.e. X cursor was hidden), it'd stay hidden until some application showed it again). The playing around with meta_stage_set_cursor etc was only to make it slightly less ugly.

Created attachment 301120 [details] [review]
nested: Allow configuration of dummy output configuration

-------

Changes since v1:

s/OUTPUT/MONITOR/ and s/output/monitor/.

Changed string split semantics (split until last delimiter, warn, and ignore if count mismatch).

Review of attachment 301120 [details] [review]:

Looks good

(In reply to Jasper St. Pierre from comment #4)
> Also, keep in mind that XFixesHideCursor/ShowCursor does not follow its
> documentation. It will hide/show the cursor *globally*, rather than on just
> the passed in window. To hide/show the cursor on just a window, use
> XDefineCursor to establish an empty cursor. We should probably do that
> instead.

Uh, wow, I wasn't really thinking too hard about what XFixesHide/ShowCursor do, but yeah, the docs do mismatch the implementation. Sent:

 http://lists.x.org/archives/xorg-devel/2015-April/046096.html  

The usage of MetaCursorTracker.set_pointer_visible() is the shell magnifier - and that *does* need to use XFixesHide/ShowCursor for the CM case - but it would be fine (and I think better) to use an empty cursor in the nested case.

To me, the easiest fix here is to make the X11 code never call XFixesHide/ShowCUrsor in the nested case, since that's essentially broken - an app has no business hiding the cursor globally.

Then to the extent you care, there's a question of having a stuck cursor overlay left around when the pointer leaves a nested window. If you wan to address that, then *that* should happen at the MetaCursorRenderer level, since the cursor overlay is not the business of the backend. 

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

> > ::: src/backends/x11/meta-cursor-renderer-x11.c
> > @@ +140,3 @@
> > +  XFixesShowCursor (xdisplay, xwindow);
> > +  priv->server_cursor_visible = TRUE;
> > +  meta_stage_set_cursor (META_STAGE (stage), NULL, &(MetaRectangle){ 0 });
> > 
> > You are really going behind the back of MetaCursorRenderer here. Just
> > because you are adding logic that is only needed in the nested case doesn't
> > mean you should add API to MetaCursorRendererX11 - what you are trying to do
> > would be much better expressed as an API such as
> > meta_cursor_renderer_set_cursor_hidden().
> 
> I suppose that could work as well, but it makes it sound like an API that is
> supposed to be used hide the cursor.
> meta_cursor_renderer_set_cursor_hidden(true) would actually just show the
> cursor on the X server, while hiding it internally, and
> _set_cursor_hidden(false) would maybe hide the X server cursor, and maybe
> show the internal cursor. Not sure that API is more any better.
> 
> Could move it to MetaCursorRenderer (keeping some left/enterd naming) and
> then make it a no-op in the native backend.

I certainly misunderstood some apsects of this code and maybe my suggestion isn't quite right, but I still think that there are likely beter ways to handle this.
 
> > For an example of why what you've done here is unrobust, if
> > meta_cursor_renderer_set_cursor() is called while the pointer is not in the
> > window, the cursor will get shown again.
> 
> It wouldn't since no client would (should at least) have pointer focus.

This seems to me logic that is complicated and implicit - we're counting on details of the Mutter pointer tracking to avoid a state where Mutter suddenly decides to hide the cursor globally. 

Also, if no client has pointer focus .... doesn't that contradict what you said below about the problem being "where the last position in mutter was over a surface" - is the actual problem that the pointer focus tracking code needs to have a state where the pointer has left the stage?

> Except if mutter/gnome-shell did it itself. Not sure it matters much, since
> this is just meant to make debugging mutter in nested mode more pleasent.

If something is just debugging, that's certainly a reason to not elaborate it and add lots of features, or to worry about obscure corner cases - but the code still has to fundamentally make sense and have robust logic - if there's code that doesn't make sense - that makes it really hard when someone else comes and needs to change the code (say they are changing something else about cursor tracking.)

> > [ I note that Using XFixesHideCursor/XFixesShowCursor is actually not
> > necessary because X cursors are inherently not displayed when the cursor is
> > not in the window. It's OK if Hide/ShowCursor happens implicitly because of
> > the implementation of set_cursor_hidden(), but it's weird to be explicitly
> > doing it here. ]
> 
> The point of this patch was really just to call
> XFixesHideCursor/XFixesShowCursor because otherwise leaving the nested
> mutter window where the last position in mutter was over a surface (i.e. X
> cursor was hidden), it'd stay hidden until some application showed it
> again). The playing around with meta_stage_set_cursor etc was only to make
> it slightly less ugly.

In what sense? In not leaving a cursor sitting in the window when the pointer leaves the stage?

(In reply to Owen Taylor from comment #10)
> (In reply to Jasper St. Pierre from comment #4)
> > Also, keep in mind that XFixesHideCursor/ShowCursor does not follow its
> > documentation. It will hide/show the cursor *globally*, rather than on just
> > the passed in window. To hide/show the cursor on just a window, use
> > XDefineCursor to establish an empty cursor. We should probably do that
> > instead.
> 
> Uh, wow, I wasn't really thinking too hard about what XFixesHide/ShowCursor
> do, but yeah, the docs do mismatch the implementation. Sent:
> 
>  http://lists.x.org/archives/xorg-devel/2015-April/046096.html  
> 
> The usage of MetaCursorTracker.set_pointer_visible() is the shell magnifier
> - and that *does* need to use XFixesHide/ShowCursor for the CM case - but it
> would be fine (and I think better) to use an empty cursor in the nested case.
> 
> To me, the easiest fix here is to make the X11 code never call
> XFixesHide/ShowCUrsor in the nested case, since that's essentially broken -
> an app has no business hiding the cursor globally.
> 
> Then to the extent you care, there's a question of having a stuck cursor
> overlay left around when the pointer leaves a nested window. If you wan to
> address that, then *that* should happen at the MetaCursorRenderer level,
> since the cursor overlay is not the business of the backend.

Fair enough.

> 
> (In reply to Jonas Ådahl from comment #7)
> 
> > > ::: src/backends/x11/meta-cursor-renderer-x11.c
> > > @@ +140,3 @@
> > > +  XFixesShowCursor (xdisplay, xwindow);
> > > +  priv->server_cursor_visible = TRUE;
> > > +  meta_stage_set_cursor (META_STAGE (stage), NULL, &(MetaRectangle){ 0 });
> > > 
> > > You are really going behind the back of MetaCursorRenderer here. Just
> > > because you are adding logic that is only needed in the nested case doesn't
> > > mean you should add API to MetaCursorRendererX11 - what you are trying to do
> > > would be much better expressed as an API such as
> > > meta_cursor_renderer_set_cursor_hidden().
> > 
> > I suppose that could work as well, but it makes it sound like an API that is
> > supposed to be used hide the cursor.
> > meta_cursor_renderer_set_cursor_hidden(true) would actually just show the
> > cursor on the X server, while hiding it internally, and
> > _set_cursor_hidden(false) would maybe hide the X server cursor, and maybe
> > show the internal cursor. Not sure that API is more any better.
> > 
> > Could move it to MetaCursorRenderer (keeping some left/enterd naming) and
> > then make it a no-op in the native backend.
> 
> I certainly misunderstood some apsects of this code and maybe my suggestion
> isn't quite right, but I still think that there are likely beter ways to
> handle this.
>  
> > > For an example of why what you've done here is unrobust, if
> > > meta_cursor_renderer_set_cursor() is called while the pointer is not in the
> > > window, the cursor will get shown again.
> > 
> > It wouldn't since no client would (should at least) have pointer focus.
> 
> This seems to me logic that is complicated and implicit - we're counting on
> details of the Mutter pointer tracking to avoid a state where Mutter
> suddenly decides to hide the cursor globally. 
> 
> Also, if no client has pointer focus .... doesn't that contradict what you
> said below about the problem being "where the last position in mutter was
> over a surface" - is the actual problem that the pointer focus tracking code
> needs to have a state where the pointer has left the stage?

If we'd unfocus the surface on leave, we'd still have the same issue for HiDPI cursors. Yes, or rather, the renderer needs to be aware if the pointer left the screen. But maybe if we can avoid to ever hide cursor globally (so that leaving the stage doesn't keep the cursor hidden), ignore that the cursor in the stage sometimes will stay visible and sometimes not, we probably don't need to have that state.

> 
> > Except if mutter/gnome-shell did it itself. Not sure it matters much, since
> > this is just meant to make debugging mutter in nested mode more pleasent.
> 
> If something is just debugging, that's certainly a reason to not elaborate
> it and add lots of features, or to worry about obscure corner cases - but
> the code still has to fundamentally make sense and have robust logic - if
> there's code that doesn't make sense - that makes it really hard when
> someone else comes and needs to change the code (say they are changing
> something else about cursor tracking.)
> 
> > > [ I note that Using XFixesHideCursor/XFixesShowCursor is actually not
> > > necessary because X cursors are inherently not displayed when the cursor is
> > > not in the window. It's OK if Hide/ShowCursor happens implicitly because of
> > > the implementation of set_cursor_hidden(), but it's weird to be explicitly
> > > doing it here. ]
> > 
> > The point of this patch was really just to call
> > XFixesHideCursor/XFixesShowCursor because otherwise leaving the nested
> > mutter window where the last position in mutter was over a surface (i.e. X
> > cursor was hidden), it'd stay hidden until some application showed it
> > again). The playing around with meta_stage_set_cursor etc was only to make
> > it slightly less ugly.
> 
> In what sense? In not leaving a cursor sitting in the window when the
> pointer leaves the stage?

Yes. Sometimes the cursor would stay visible (if it was drawn as a stage overlay), sometimes it'd not (if it was drawn by the server). Its not an important thing to care about really, so can just ignore that detail.

Created attachment 301174 [details] [review]
nested: Allow configuration of dummy output configuration

Enable a user to test and debug multi output configurations on Wayland
without having the available hardware by enabling some basic
configuration of the dummy monitor manager.

Currently available configuration options are:

MUTTER_DEBUG_NUM_DUMMY_MONITORS - to set the number of monitors
MUTTER_DEBUG_DUMMY_MONITOR_SCALES - to configure the monitor scales

See src/backends/meta-monitor-manager-dummy.c for detailed description
of the available configuration parameters.

Created attachment 301175 [details] [review]
nested: Use XDefineCursor to hide server cursor

Set a generated empty cursor to hide the cursor when running in nested
compositor mode. We should not use XFixes for this, since XFixes hides
the parent display server cursor globally, which means that if the
pointer leaves the compositor stage when the cursor was hidden it'd stay
hidden.

The only change to the configuration patch was the commit message.

Review of attachment 301175 [details] [review]:

Looks good to commit except for some XFlush calls that you seem to have propagated from elsewhere in the Mutter source code (introduced in commit 8ce054b2 by Matthias?), but are not needed.

::: src/backends/x11/meta-cursor-renderer-x11.c
@@ +53,3 @@
+      Cursor xcursor = meta_cursor_create_x_cursor (xdisplay, cursor);
+      XDefineCursor (xdisplay, xwindow, xcursor);
+      XFlush (xdisplay);

this flush is not needed

@@ +63,3 @@
+#ifdef HAVE_WAYLAND
+static Cursor
+create_empty_cursor (Display *xdisplay)

I'd probably have cached an empty cursor, but it's not going to make a bit of observable difference and is only for the testing case anyways, so fine like this :-)

@@ +97,3 @@
+      Cursor empty_xcursor = create_empty_cursor (xdisplay);
+      XDefineCursor (xdisplay, xwindow, empty_xcursor);
+      XFlush (xdisplay);

This one isn't needed either

(In reply to Owen Taylor from comment #15)
> Review of attachment 301175 [details] [review] [review]:
> 
> Looks good to commit except for some XFlush calls that you seem to have
> propagated from elsewhere in the Mutter source code (introduced in commit
> 8ce054b2 by Matthias?), but are not needed.
> 
> ::: src/backends/x11/meta-cursor-renderer-x11.c
> @@ +53,3 @@
> +      Cursor xcursor = meta_cursor_create_x_cursor (xdisplay, cursor);
> +      XDefineCursor (xdisplay, xwindow, xcursor);
> +      XFlush (xdisplay);
> 
> this flush is not needed
> 
> @@ +63,3 @@
> +#ifdef HAVE_WAYLAND
> +static Cursor
> +create_empty_cursor (Display *xdisplay)
> 
> I'd probably have cached an empty cursor, but it's not going to make a bit
> of observable difference and is only for the testing case anyways, so fine
> like this :-)

I actually did, with a static variable in the function, but removed it for exactly that reason.

> 
> @@ +97,3 @@
> +      Cursor empty_xcursor = create_empty_cursor (xdisplay);
> +      XDefineCursor (xdisplay, xwindow, empty_xcursor);
> +      XFlush (xdisplay);
> 
> This one isn't needed either

Ok, I'll drop the XFlush:ing.

Created attachment 301249 [details] [review]
nested: Use XDefineCursor to hide server cursor

Set a generated empty cursor to hide the cursor when running in nested
compositor mode. We should not use XFixes for this, since XFixes hides
the parent display server cursor globally, which means that if the
pointer leaves the compositor stage when the cursor was hidden it'd stay
hidden.

-----

The previous version actually had a severe flaw: it would never call
XDefineCursor when running as a X CM. This patch restore the correct
semantics. Please have a look again.

Review of attachment 301249 [details] [review]:

Looks like it will work, but on the other hand, it's pretty confusing - a comment I should have made about the last patch, I guess!

I think the show_server_cursor()/hide_server_cursor() functions are not adding any useful abstraction but rather confusing things and the logic needs to be moved back into meta_cursor_renderer_x11_update_cursor(). Maybe something like:

    set_server_cursor (backend, cursor);
    if (meta_wayland_is_compositor()) 
      {
       /* When we are running nested, we control server cursor visibility
        * by setting an empty cursor, so setting a non-empty cursor in
        * itself shows the server cursor */
       priv->server_cursor_visible = TRUE;
     }
    has_server_cursor = TRUE;

[...]

 if (has_server_cursor != priv->server_cursor_visible) 
  {
    if (meta_wayland_is_compositor()) 
      {      
         g_assert(!has_server_cursor); /* otherwise priv->server_cursor_visible was adjusted above */
         set_empty_cursor()
       }
    else
      {    
        if (has_server_cursor)
	  XFixesShowCursor (xdisplay, xwindow);
        else
	  XFixesHideCursor (xdisplay, xwindow);
       }
   }

(In reply to Owen Taylor from comment #18)
> Review of attachment 301249 [details] [review] [review]:
> 
> Looks like it will work, but on the other hand, it's pretty confusing - a
> comment I should have made about the last patch, I guess!
> 
> I think the show_server_cursor()/hide_server_cursor() functions are not
> adding any useful abstraction but rather confusing things and the logic
> needs to be moved back into meta_cursor_renderer_x11_update_cursor(). Maybe
> something like:
> 
>     set_server_cursor (backend, cursor);
>     if (meta_wayland_is_compositor()) 
>       {
>        /* When we are running nested, we control server cursor visibility
>         * by setting an empty cursor, so setting a non-empty cursor in
>         * itself shows the server cursor */
>        priv->server_cursor_visible = TRUE;
>      }
>     has_server_cursor = TRUE;
> 
> [...]
> 
>  if (has_server_cursor != priv->server_cursor_visible) 
>   {
>     if (meta_wayland_is_compositor()) 
>       {      
>          g_assert(!has_server_cursor); /* otherwise
> priv->server_cursor_visible was adjusted above */
>          set_empty_cursor()
>        }
>     else
>       {    
>         if (has_server_cursor)
> 	  XFixesShowCursor (xdisplay, xwindow);
>         else
> 	  XFixesHideCursor (xdisplay, xwindow);
>        }
>    }

Hmm. I'm not sure I understand the intention of your proposal. Changing the visibility surface of the server cursor is done either when changing from a cursor created from a theme and a cursor created from pixels (wl_buffer or texture), or when going from or to scale 1. It doesn't have anything to do with whether we are running as a Wayland compositor or not. The only difference depending on whether we're running as a X CM or a Wayland compositor is *how* we hide the cursor.

With the code above, we'd also never enter the if (has_server_cursor != priv->server_cursor_visible) branch since you change the state up front.

(In reply to Jonas Ådahl from comment #19)
> (In reply to Owen Taylor from comment #18)
> > Review of attachment 301249 [details] [review] [review] [review]:
> > 
> > Looks like it will work, but on the other hand, it's pretty confusing - a
> > comment I should have made about the last patch, I guess!
> > 
> > I think the show_server_cursor()/hide_server_cursor() functions are not
> > adding any useful abstraction but rather confusing things and the logic
> > needs to be moved back into meta_cursor_renderer_x11_update_cursor(). Maybe
> > something like:
> > 
> >     set_server_cursor (backend, cursor);
> >     if (meta_wayland_is_compositor()) 
> >       {
> >        /* When we are running nested, we control server cursor visibility
> >         * by setting an empty cursor, so setting a non-empty cursor in
> >         * itself shows the server cursor */
> >        priv->server_cursor_visible = TRUE;
> >      }
> >     has_server_cursor = TRUE;
> > 
> > [...]
> > 
> >  if (has_server_cursor != priv->server_cursor_visible) 
> >   {
> >     if (meta_wayland_is_compositor()) 
> >       {      
> >          g_assert(!has_server_cursor); /* otherwise
> > priv->server_cursor_visible was adjusted above */
> >          set_empty_cursor()
> >        }
> >     else
> >       {    
> >         if (has_server_cursor)
> > 	  XFixesShowCursor (xdisplay, xwindow);
> >         else
> > 	  XFixesHideCursor (xdisplay, xwindow);
> >        }
> >    }
> 
> Hmm. I'm not sure I understand the intention of your proposal. Changing the
> visibility surface of the server cursor is done either when changing from a
> cursor created from a theme and a cursor created from pixels (wl_buffer or
> texture), or when going from or to scale 1. It doesn't have anything to do
> with whether we are running as a Wayland compositor or not. The only
> difference depending on whether we're running as a X CM or a Wayland
> compositor is *how* we hide the cursor.

Sure, that's my understanding.

> With the code above, we'd also never enter the if (has_server_cursor !=
> priv->server_cursor_visible) branch since you change the state up front.

Only in the case I assert that you don't enter that branch, right?

Anyways, the heart of of my comment is that having a show_server_cursor() function that sometimes does what it says, and sometimes does nothing because the cursor is assumed to already have been shown is bad practice.

- Owen

(In reply to Owen Taylor from comment #20)
> (In reply to Jonas Ådahl from comment #19)
> > (In reply to Owen Taylor from comment #18)
> > > Review of attachment 301249 [details] [review] [review] [review] [review]:
> > > 
> > > Looks like it will work, but on the other hand, it's pretty confusing - a
> > > comment I should have made about the last patch, I guess!
> > > 
> > > I think the show_server_cursor()/hide_server_cursor() functions are not
> > > adding any useful abstraction but rather confusing things and the logic
> > > needs to be moved back into meta_cursor_renderer_x11_update_cursor(). Maybe
> > > something like:
> > > 
> > >     set_server_cursor (backend, cursor);
> > >     if (meta_wayland_is_compositor()) 
> > >       {
> > >        /* When we are running nested, we control server cursor visibility
> > >         * by setting an empty cursor, so setting a non-empty cursor in
> > >         * itself shows the server cursor */
> > >        priv->server_cursor_visible = TRUE;
> > >      }
> > >     has_server_cursor = TRUE;
> > > 
> > > [...]
> > > 
> > >  if (has_server_cursor != priv->server_cursor_visible) 
> > >   {
> > >     if (meta_wayland_is_compositor()) 
> > >       {      
> > >          g_assert(!has_server_cursor); /* otherwise
> > > priv->server_cursor_visible was adjusted above */
> > >          set_empty_cursor()
> > >        }
> > >     else
> > >       {    
> > >         if (has_server_cursor)
> > > 	  XFixesShowCursor (xdisplay, xwindow);
> > >         else
> > > 	  XFixesHideCursor (xdisplay, xwindow);
> > >        }
> > >    }
> > 
> > Hmm. I'm not sure I understand the intention of your proposal. Changing the
> > visibility surface of the server cursor is done either when changing from a
> > cursor created from a theme and a cursor created from pixels (wl_buffer or
> > texture), or when going from or to scale 1. It doesn't have anything to do
> > with whether we are running as a Wayland compositor or not. The only
> > difference depending on whether we're running as a X CM or a Wayland
> > compositor is *how* we hide the cursor.
> 
> Sure, that's my understanding.
> 
> > With the code above, we'd also never enter the if (has_server_cursor !=
> > priv->server_cursor_visible) branch since you change the state up front.
> 
> Only in the case I assert that you don't enter that branch, right?

I see. You avoid potential future branch of (is_visible != was_visible), but only when going was_visible -> is_visible. I don't think that is more clear. How about we do what you do above, but without the up front state change, and instead just change the assert to if (!has_server_cursor)? No sneaky state changes and if (was != is) still makes sense.

> 
> Anyways, the heart of of my comment is that having a show_server_cursor()
> function that sometimes does what it says, and sometimes does nothing
> because the cursor is assumed to already have been shown is bad practice.
> 
> - Owen

(In reply to Jonas Ådahl from comment #21)
> > Only in the case I assert that you don't enter that branch, right?
> 
> I see. You avoid potential future branch of (is_visible != was_visible), but
> only when going was_visible -> is_visible. I don't think that is more clear.
> How about we do what you do above, but without the up front state change,
> and instead just change the assert to if (!has_server_cursor)? No sneaky
> state changes and if (was != is) still makes sense.

Sure - that's fine - I was trying to avoid temporarily having an inconsistent state where priv->server_cursor_visible is FALSE and yet the server cursor is visible, but as long as that inconsistent state only exists inside one function and the set-cursor-shows-cursor side effect is commented, no problem.

Created attachment 301289 [details] [review]
nested: Use XDefineCursor to hide server cursor

Set a generated empty cursor to hide the cursor when running in nested
compositor mode. We should not use XFixes for this, since XFixes hides
the parent display server cursor globally, which means that if the
pointer leaves the compositor stage when the cursor was hidden it'd stay
hidden.

Review of attachment 301289 [details] [review]:

One problem, otherwise OK to commit

::: src/backends/x11/meta-cursor-renderer-x11.c
@@ +115,3 @@
+          if (!has_server_cursor)
+            {
+              Cursor empty_xcursor = create_empty_cursor (xdisplay);

The function is inside #ifdef HAVE_WAYLAND, but this isn't

Comment on attachment 301174 [details] [review]
nested: Allow configuration of dummy output configuration

Attachment 301174 [details] pushed as bede997 - nested: Allow configuration of dummy output configuration