Created attachment 153193 [details] [review]
[Overview] Fix memory leak when activating a zoomed window

While zooming, a window clone is reparented to the stage and thus cannot
be destroyed automatically by the "parent" container.
To prevent the clone and its corresponding overlay from leaking, destroy
the clone manually when the original parent is destroyed during zooming.

(In reply to comment #0)
> When activating a zoomed window clone in the window picker, the following
> error appears:
> 
> (mutter:32539): Clutter-WARNING **: Actor of type ClutterClone is not in the
> same container of actor of type ClutterClone
> 
> This is an indication that the workspace container fails to correctly destroy
> the clone.

For reference this unclear warning is probably coming from the ClutterActor.raise() call on zoomEnd. (I filed http://bugzilla.openedhand.com/show_bug.cgi?id=1977)

Review of attachment 153193 [details] [review]:

I think rather than using signal connections it would be better to handle the case of leaving the overview (selecting a zoomed window or hot key) explicitly.

*** Bug 609356 has been marked as a duplicate of this bug. ***

Retitling to handle slightly wider scope of the bug I duplicated on this bug.

Created attachment 153317 [details] [review]
Fix problems when leaving the overview with a zoomed window

OKay, not sure if I understood you correctly, but here we go ...

Review of attachment 153317 [details] [review]:

Hmm, think this is closer, but maybe still needs a bit of work.

::: js/ui/workspace.js
@@ +132,3 @@
+            function() {
+                this._leavingOverview = true;
+            }));

Rather than a forest of signal connections, I think it's cleaner if it's something like:

 _getWorkspace: function() {
    ....
 }

 if (_getWorkspace().leavingOverview) {
 }

Or maybe the workspace should be passed to the window clone in the constructor and updated when a window gets moved between workspaces.

@@ +151,1 @@
         this.actor.destroy();

We generally want to avoid adding functionality to destroy() methods - it should work correctly if the actor gets destroyed as a consequence of the actors parent getting destroyed. See bug 609454. So an _onDestroy signal connection should be added instead.

@@ +171,3 @@
+        if (this._zoomStep) {
+            if (this._leavingOverview)
+                this.destroy();

We definitely don't want to destroy in this case - that will mean the window will go poof, and reappear when we get back to the desktop. Probably the right thing to do if user hits escape or the windows key is to cancel the zoom when starting hiding the overview. If the user actually clicks on the window to select it, then we should act as we currently do and zoom into the window, without an intermediate step of snapping back to the old position.

Created attachment 154129 [details] [review]
Cancel zoom of non-focused windows when leaving overview

(In reply to comment #7)
> Probably the right thing to do if user hits escape or the windows key is
> to cancel the zoom when starting hiding the overview. If the user actually
> clicks on the window to select it, then we should act as we currently do
> and zoom into the window, without an intermediate step of snapping back to
> the old position.

Sounds reasonable - I'll attach the patch here, although it's probably a separate issue.

Created attachment 154141 [details] [review]
Fix problems when leaving the overview with a zoomed window

(In reply to comment #7)
> Rather than a forest of signal connections, I think it's cleaner if it's
> something like:
> 
>  _getWorkspace: function() {
>     ....
>  }
> 
>  if (_getWorkspace().leavingOverview) {
>  }
> 
> Or maybe the workspace should be passed to the window clone in the constructor
> and updated when a window gets moved between workspaces.

I tried that, but couldn't get it to work - the problem was that workspace.leavingOverview was always set to false when I queried it. I found a solution which worked without signal handlers altogether (in _zoomEnd(): add a check for this._stackAbove.get_parent() before the raise() call and destroy the clone if _stackAbove has been unparented), but after a conversation with ebassi on IRC I decided that this was way too hackish - it relies on a leave-event we receive (for whatever reason) when the overview is left (the handler calls _zoomEnd(), and there the call to raise() triggers the warning).  The real problem is that the WindowClone is not destroyed with the Workspace - addressing this from WindowClone is messy (see first patch version), so I propose doing it from Workspace instead.

The patch sets up a signal handler for each clone - alternatively, the same could be done iterating over the clones in the (yet to be written) _onDestroy() handler for the workspace.

Thoughts?

Comment on attachment 154141 [details] [review]
Fix problems when leaving the overview with a zoomed window

(mutter:19107): Clutter-WARNING **: Actor of type ClutterClone is not in the sa
me container of actor of type ClutterClone
Window manager warning: Log level 16: gsignal.c:2387: instance `0x88c8938' has 
no handler with id `1424'
Window manager warning: Log level 16: gsignal.c:2387: instance `0x88c8938' has 
no handler with id `1425'
Window manager warning: Log level 16: gsignal.c:2387: instance `0x88c8938' has 
no handler with id `1426'
Window manager warning: Log level 16: gsignal.c:2387: instance `0x8fbc160' has 
no handler with id `1423'

Now testing the other approach outlined above ...

Created attachment 154164 [details] [review]
Fix problems when leaving the overview with a zoomed window

Destroy all window clones in the _onDestroy() handler of the workspace (well, the patch actually uses the destroy() method, but it does not conflict with bug 609454 - applying that patch should do the right thing ...)

Created attachment 154384 [details] [review]
Fix problems when leaving the overview with a zoomed window

The previous patch messed up zooming of newly added / moved windows.

agree with Owen's earlier comment that we shouldn't be connecting to Main.overview's signals from every clone. We already iterate over each clone when exiting the overview (to animate them all back into place), so we can just call an "unzoom" method on each one at the same time, right?

Created attachment 154436 [details] [review]
Cancel zoom of non-focused windows when leaving overview

(In reply to comment #13)
> We already iterate over each clone when exiting the overview (to animate
> them all back into place), so we can just call an "unzoom" method on each
> one at the same time, right?

Sure.

Review of attachment 154384 [details] [review]:

Assuming that the comment I asked for is along the lines of:

 "Usually, the windows will be destroyed automatically with the workspace parent, but sometimes we might have a zoomed window which has been reparented to the stage"

Then this is fine to commit with comment fixes. Will review the other patch now.

::: js/ui/workspace.js
@@ +236,3 @@
         this.actor.reparent(this._origParent);
+        // If the workspace has been destroyed while we were reparented to
+        // the stage, the desktop will be unparented and we can't raise our

stackAbove isn't always the desktop, is it? It's typically another window clone if I'm not mistaken.

@@ +1355,3 @@
+
+        for (let w = 1; w < this._windows.length; w++)
+            this._windows[w].destroy();

This needs a comment explaining why you are doing this, and another reminder the reader that windows[0] is the desktop window (and probably should explain why its special here)

Review of attachment 154436 [details] [review]:

Looks good to commit, except for some rewriting for clarity.

(for the commit message, 'focussed' => 'focused')

::: js/ui/workspace.js
@@ +152,3 @@
+        let focus_window = global.screen.get_display().focus_window;
+        if (focus_window != this.metaWindow)
+            this._zoomEnd();

Needs some rewriting from clarity - something like

 if (this._zooming) {
     // If the user clicked on the zoomed window, or we are
     // returning there anyways, then we can zoom right to the
     // window, but if we are going to some other window, then
     // we need to cancel the zoom before animating, or it
     // will look funny.

     if (!this._selected &&
         this.metaWindow != global.screen.get_display().focus_window)
         this.zoomEnd();
 }

So, basically exactly the same as what you have but with an emphasis on the *why* - with the way you wrote it, it's unclear that this._selected and focus_window == this.metaWindow are the same case.

Attachment 154384 [details] pushed as 5429104 - Fix problems when leaving the overview with a zoomed window
Attachment 154436 [details] pushed as a21ba29 - Cancel zoom of non-focused windows when leaving overview