Created attachment 136726 [details] [review]
Add better tracking of real stacking order

Wedging override-redirect windows into the constraint code in stack.c
results in Mutter getting confused about the stacking order of
these windows with respect to other windows, and may also in some
cases cause Mutter to restack override-redirect windows.

core/stack-tracker.c core/stack-tracker.h: MetaStackTracker - combine
  events received from the X server with local changes we have made
  to come up with the best possible idea of what the stacking order
  is at any one point in time.

core/screen.c core/screen-private.h: Create a MetaStackTracker for
  the screen.

core/display.c: Feed relevant events to MetaStackTracker

core/frame.c core/screen.c core/stack.c: When we make changes to the
  stacking order or add windows, record those changes immediatley
  in MetaStackTracker so we have the information without waiting
  for a round-trip.

include/ui.h ui/ui.c: meta_ui_create_frame_window add a return value
  for the X request serial used to create the window.

Created attachment 136727 [details] [review]
Move compositor-stack handling to MetaStackTracker

In order to properly track the stacking order for override-redirect
windows, move meta_compositor_sync_stack() call into MetaStackTracker.
In the new location, we sync the stack as a before-redraw idle function,
rather then using the freeze-thaw facilities of MetaStack. This is
simpler, and also properly compresses multiple stack changes on
notifications received from the X server.

Created attachment 136728 [details] [review]
Don't do stacking for override-redirect windows

Don't add override-redirect windows to MetaStack; we shouldn't
be restacking them.

Since we *aren't* stacking the override-redirect windows, we need to
be careful that to ignore them when looking for the top managed
window.

Created attachment 136729 [details] [review]
Use MetaStackTracker to avoid a round-trip XQueryTree()

With MetaStackTracker, it's no longer necessary to XQueryTree to
get a reasonably-up-to-date view of the server stacking order.

Add some comments explaining unclear aspects of
raise_window_relative_to_managed_windows() and with future possible
improvements.

I've pushed the patches from:

 Bug 582639 – should not move/resize override redirect windows
 Bug 585984 – Fix stacking for override-redirect windows

along with some other separately filed bug fixes that these patches depend upon to the 'override-redirect-exclusion' branch in the Mutter repository.

Created attachment 136853 [details] [review]
Fix shadowing problem breaking MetaStackTracker

(In reply to comment #1)
> Created an attachment (id=136726) [edit]
> Add better tracking of real stacking order

+  meta_stack_tracker_record_add (screen->stack_tracker,
+                                 guard_window,
+                                 XNextRequest(xdisplay) - 1);
+
   XLowerWindow (xdisplay, guard_window);

Is the "- 1" to reflect the fact that you aren't calling meta_stack_tracker_record_lower()? And if so, why?

+  /* We make an assumption that gdk_window_new() is going to call
+   * XCreateWindow as it's first operation; this seems to be true currently
+   * as long as you pass in a colormap.
+   */
+  if (create_serial)
+    *create_serial = XNextRequest (xdisplay);

It seems like if create_serial is too low, then stack_tracker_event_received() could potentially remove the STACK_OP_ADD from the queue before receiving the CreateEvent, and so it would temporarily lose track of the window. Whereas if it was too high, the op might get left in the queue too long, which would cause an "already in stack" g_warning, but nothing else. So it would be safer to set create_serial based on what XNextRequest returns *after* creating the window.

The meta_stack_tracker_get_stack() docs don't say which order the stack is in.

(In reply to comment #2)
> Created an attachment (id=136727) [edit]
> Move compositor-stack handling to MetaStackTracker

Good

(In reply to comment #3)
> Created an attachment (id=136728) [edit]
> Don't do stacking for override-redirect windows

+  if (!window->override_redirect)
+    meta_stack_add (window->screen->stack,
+                    window);
+  else
+    window->layer = META_LAYER_OVERRIDE_REDIRECT; /* otherwise set by MetaStack */

is there any reason for META_LAYER_OVERRIDE_REDIRECT to still exist?

(In reply to comment #4)
> Created an attachment (id=136729) [edit]
> Use MetaStackTracker to avoid a round-trip XQueryTree()

good

(In reply to comment #7)
> (In reply to comment #1)
> > Created an attachment (id=136726) [edit]
> > Add better tracking of real stacking order
> 
> +  meta_stack_tracker_record_add (screen->stack_tracker,
> +                                 guard_window,
> +                                 XNextRequest(xdisplay) - 1);
> +
>    XLowerWindow (xdisplay, guard_window);
> 
> Is the "- 1" to reflect the fact that you aren't calling
> meta_stack_tracker_record_lower()? And if so, why?

No, the -1 is because we have to call record *after* CreateWindow (since otherwise we don't have the XID yet.) So, the request for CreateWindow is 1 less than the next request. Or a temporary variable could be used to save the create serial. Probably needs a comment in any case.

Yes, record_lower() should be called here, though it doesn't end up mattering a lot in practice. Good catch.

> +  /* We make an assumption that gdk_window_new() is going to call
> +   * XCreateWindow as it's first operation; this seems to be true currently
> +   * as long as you pass in a colormap.
> +   */
> +  if (create_serial)
> +    *create_serial = XNextRequest (xdisplay);
> 
> It seems like if create_serial is too low, then stack_tracker_event_received()
> could potentially remove the STACK_OP_ADD from the queue before receiving the
> CreateEvent, and so it would temporarily lose track of the window. Whereas if
> it was too high, the op might get left in the queue too long, which would cause
> an "already in stack" g_warning, but nothing else. So it would be safer to set
> create_serial based on what XNextRequest returns *after* creating the window.

The reason I went with the ID before is that there is a lot more random code that could trigger X events gtk+/gdk/x11/gdkwindow-x11.c:_gdk_window_new() after the CreateWindow than before. I'm not sure if any of that is actually hit, but I felt more comfortable with deriving the serial for CreateWIndow with the serial before. Could use a comment.

> The meta_stack_tracker_get_stack() docs don't say which order the stack is in.

Will add.

> (In reply to comment #3)
> > Created an attachment (id=136728) [edit]
> > Don't do stacking for override-redirect windows
> 
> +  if (!window->override_redirect)
> +    meta_stack_add (window->screen->stack,
> +                    window);
> +  else
> +    window->layer = META_LAYER_OVERRIDE_REDIRECT; /* otherwise set by
> MetaStack */
> 
> is there any reason for META_LAYER_OVERRIDE_REDIRECT to still exist?

Well, window->layer needs to be something, so having a LAYER_OVERRIDE_REDIRECT makes some sense, even if it means "not in any layer". Removing LAYER_OVERRIDE_REDIRECT breaks gnome-shell as well, which is the main reason I didn't go ahead and try to eliminate it.

Created attachment 137457 [details] [review]
Avoid restacking animating hidden actors

the X stacking order really accurately, hidden windows in
live_hidden_windows mode are accurately pushed to the bottom
of the stack; meaning that a minimizing window animates below
everything else.

Here's a patch that works around this by avoiding restacking the
actors for such windows.

Since we don't consider windows being animated when switching desktops
to be "effect in progress" windows, this patch doesn't do anything for
them.  However:

 - The stacking order of the windows *between* the desktops doesn't
   really matter. And all the windows on one of the desktops should
   be hidden and all the windows on the other not.

 - Whether using the clone or reparenting approach, the stacking order
   of the original actors is not what is visible to the user anyways.

There are certainly some weirdness with stacking order when switching
desktops in regards to sticky and override-redirect windows, but I
don't think there's any regression - the same or similar problems occur
in master.

(In reply to comment #9)
> 
> the X stacking order really accurately, hidden windows in
> live_hidden_windows mode are accurately pushed to the bottom
> of the stack; meaning that a minimizing window animates below
> everything else.

A few words got lost there - was meant to say:

Now we model the X stacking order really accurately, hidden windows in live_hidden_windows mode are accurately pushed to the bottom  of the stack; meaning that a minimizing window animates below everything else.
 

+      stack = g_list_remove (stack, window);
+      old_stack = g_list_remove (old_stack, actor);

That should be done inside the if/else bodies, since the "stack = ..." will be a slow no-op in the "else" case and the "old_stack = ..." will be a slow no-op in the "if" case.

(In reply to comment #11)
> +      stack = g_list_remove (stack, window);
> +      old_stack = g_list_remove (old_stack, actor);
> 
> That should be done inside the if/else bodies, since the "stack = ..." will be
> a slow no-op in the "else" case and the "old_stack = ..." will be a slow no-op
> in the "if" case.

The window is likely present in the both stacks, though it might not be in the front of both of them. Not removing the window from both stacks will result in windows ending up in the result list multiple times.

(Hopefully the two stacks are reasonably similar, in which case we are usually removing one of the first few elements of the list and it isn't too slow.)

Might need some more commenting :-)

I'm still some issues where sometimes a minimizing window gets lowered, so I need to debug a little more what's going on before I commit. (Theory: the focus gets switched to a new window and it gets restacked above before the minimize effect is started. May be specific to my effect-rework patches.)

Created attachment 137617 [details] [review]
Fix confusion from old previously-hidden windows in the stack

If the first restack after a minimize animation is another minimize
animation, then the old actor is still "suspended" in our local
copy of the stack; we need to ignore such actors or the logic
for keeping the currently animating actor from being restacked
will be defeated.

The last attached was the bug fix to the previous patch necessary to fully fix minimizing windows; I've 

 - squashed it with the previous patch
 - made the improvements suggested above
 - merged everything to master