Created attachment 144214 [details] [review]
[workspaces] Sync clone stacking order with MetaStackTracker

Previously, we initialized actor stacking order from the return
value of global.get_windows() once, which is defined to be in
stack order.  However it was not updated later.  Furthermore,
the way stackAbove was called from onAnimationComplete in
WindowClone was highly dubious, since there are lots of animations
which apply to the clones, and we want the stacking to be right
all of the time, not when some animation completes.

Fix this by connecting to 'restacked' on the screen and syncing
the clones.

I also snuck in another bugfix here; we weren't disconnecting
from the 'hiding' signal handler, which had various bad
consequences.

Review of attachment 144214 [details] [review]:

Code here looks good - only minor identation comments mostly. 

Couple of things I don't think you are handling:

 - there's a call to actor.raise_top in WindowClone.onEnter that I don't see you dealing with here. That call was originally added I think for something dealing with DND - not sure. Things seem to work OK for it's commented out so it probably can be removed.

 - When the scroll-wheel zooming is used, we reparent the clone out to the stage, then reparent it back, which is going to disturb the stacking order

 - snap back for DND doesn't make any attempt to restore the stacking order when reparenting the drag order back to its parent



But that

::: js/ui/workspaces.js
@@ +850,3 @@
     },
 
+    _getVisibleWindows: function() {

Needs a comment brief comment that it returns an array of MetaWindow - I was wondering looking at it, why it wasn't the same as:

 if (this._showOnlyWindows ! null)
    return this._showOnlyWindows;
 else
    return this._windows;

(other than making a copy), but this._showOnlyWindows is MetaWindow and this._windows is WindowClones.

@@ +1353,1 @@
         }));

This })) should have been moved in 4 spaces

@@ +1629,3 @@
+        for (let i = 0; i < stack.length; i++) {
+            stackIndices[stack[i].get_meta_window()] = i;
+        }

I'm not obsessive about excess {} for single line blocks, but it looks really ugly to have it here since there is a single-line for loop without it immediately below.

(In reply to comment #2)
>  - there's a call to actor.raise_top in WindowClone.onEnter that I don't see
> you dealing with here. That call was originally added I think for something
> dealing with DND - not sure. Things seem to work OK for it's commented out so
> it probably can be removed.

Actually, it was for when we fell back from a grid layout to the overlapping layout, so that when you moused over a window you could see the whole thing. Should be unnecessary now, yeah.

Created attachment 144302 [details] [review]
Sync clone stacking order with MetaStackTracker

Previously, we initialized actor stacking order from the return
value of global.get_windows() once, which is defined to be in
stack order.  However it was not updated later.  Furthermore,
the way stackAbove was called from onAnimationComplete in
WindowClone was highly dubious, since there are lots of animations
which apply to the clones, and we want the stacking to be right
all of the time, not when some animation completes.

Fix this by connecting to 'restacked' on the screen and syncing
the clones.

I also snuck in another bugfix here; we weren't disconnecting
from the 'hiding' signal handler, which had various bad
consequences.

Comment on attachment 144302 [details] [review]
Sync clone stacking order with MetaStackTracker

>I also snuck in another bugfix here; we weren't disconnecting
>from the 'hiding' signal handler, which had various bad

'showing', actually

>+        // We'll fix up the stack after the drag/zooming
>+        if (this._inDrag || this._zooming)
>+            return;

Style nit: I think it's confusing to have a comment *before* an if that says something that's only true *inside* the if. (So I'd either move the comment to inside the if body, or change it to say "If we're dragging/zooming, we'll fix up the stack later".) YMMV.

>+        if (this._stackAbove != null)
>+            this.actor.raise(this._stackAbove);

stackAbove should always be non-null at this point (_zoomEnd), I think.

>+        // We may not have a parent if DnD completed successfully, in
>+        // which case our clone will shortly be destroyed and replaced
>+        // with a new one on the target workspace.
>+        if (this._stackAbove != null && this.actor.get_parent() != null)

likewise here

>+    _getVisibleClonesAndIndices: function() {

you don't seem to use this method anywhere except in _getVisibleClones and _getVisibleWindows, neither of which cares about the indices.

>+            stackIndices[stack[i].get_meta_window()] = i;

Ew... are you aware that that's equivalent to:

              stackIndices[stack[i].get_meta_window().toString()] = i;

? It only works at all because we changed gjs's toString to include "%p", but it seems a little sketchy to depend on the fact that toString gives a result which is both unique and persistent (eg, if you added a debug option to show the ref_count or something in toString, then that would break any code that used it as a hash key).

Attachment 144302 [details] pushed as 20b29ff - Sync clone stacking order with MetaStackTracker