Created attachment 153963 [details] [review]
Don't destroy and recreate workspaces on view switches

Currently, the workspace objects are destroyed and recreated on
view switches. Instead, keep the objects around and reparent the
workspace actors on view switches.

Created attachment 153964 [details] [review]
Animate workspace view switches

Replace the hard switch between linear and mosaic workspace view
with a an animated transition between the views.

Created attachment 154096 [details] [review]
Don't destroy and recreate workspaces on view switches

Rebased patch.

Created attachment 154097 [details] [review]
Animate workspace view switches

Rebased patch with minor stylistic changes.

Created attachment 154450 [details] [review]
Don't destroy and recreate workspaces on view switches

Rebased patch.

Comment on attachment 154450 [details] [review]
Don't destroy and recreate workspaces on view switches

This is a good idea, but the current patch makes the handling of the workspaces array quite bizarre: WorkspacesControls creates the initial workspace view and passes it an empty array, which the view then fills in. The view takes care of updating the array as workspaces are added and removed, and then, if the user switches views, WorkspacesControls destroys the original view and passes the workspaces array to the new view, taking advantage of the fact that arrays are pass-by-reference, and so its copy of the array has any changes that the original view made.

If WorkspacesControls is going to "own" the array, then it should update it itself when n-workspaces changes, and then call an appropriate method on the view to let it reflect the change.

Related to that, it's a bit odd that the delegate for the toolbar is now essentially the "controller" object for the entire workspace area. It would probably be cleaner to split out a WorkspacesManager that owns both the control bar and the view.

Comment on attachment 154097 [details] [review]
Animate workspace view switches

first, the fact that the workspaces are now animated makes the appearance/disappearance of the scrollbar look abrupt and strange. So you should tween that too. (Either tween its opacity, or slide it in/out of view off the bottom of the workspaces area.)

>-                this._positionWorkspaces();
>+                if (this._transitionToOverview)
>+                    this._positionWorkspaces();
>+                else
>+                    this._transitionWorkspaces();

gah. why is this happening from style-changed?

>+            Tweener.addTween(workspace.actor, {
>+                             x: workspace.gridX,

fix indentation style


I didn't review this in detail, because it's going to end up needing to change based on the problems with the other patch anyway

Created attachment 155510 [details] [review]
Don't destroy and recreate workspaces on view switches

(In reply to comment #6)
> (From update of attachment 154450 [details] [review])
> This is a good idea, but the current patch makes the handling of the workspaces
> array quite bizarre: WorkspacesControls creates the initial workspace view and
> passes it an empty array, which the view then fills in.

Even more bizarre: the empty arrays is used to indicate to the view that it does not need to animate a view switch. Eeek. Fixed.


> Related to that, it's a bit odd that the delegate for the toolbar is now
> essentially the "controller" object for the entire workspace area. It would
> probably be cleaner to split out a WorkspacesManager that owns both the control
> bar and the view.

Kind of done - it might be better to add an actor property to the manager, which contains both view and control bar actors.

Created attachment 155513 [details] [review]
Animate workspace view switches

(In reply to comment #7)
> first, the fact that the workspaces are now animated makes the
> appearance/disappearance of the scrollbar look abrupt and strange. So you
> should tween that too.

I agree, I just couldn't think of a general way of doing it. The attached patch relies on the fact that one of the views returs null.


> gah. why is this happening from style-changed?

The spacing between workspaces is taken from CSS, both positionWorkspaces() and transitionWorkspaces() need to take this into account to position the workspaces correctly. Would it be less ugly to something like

_init:
    this._transitionNeeded = !Main.overview.animationInProgress;
    ...
_positionWorkspaces:
    ...
    if (this._transitionNeeded) {
        this._transitionWorkspaces();
        this._transitionNeeded = false;
    }


> >+            Tweener.addTween(workspace.actor, {
> fix indentation style
Fixed? There are two styles in the file, updated patch to use the "other" one ...

Created attachment 156293 [details] [review]
Don't destroy and recreate workspaces on view switches

Rebased patch - this patch creates a dependency on bug 610892 due to the way it handles the connect/disconnect of the window-drag-begin/window-drag-end signals; it would not be too hard to create independent patch, but as I expect the other one to land before this one, it was the easiest thing to do ...

Created attachment 156294 [details] [review]
Animate workspace view switches

Rebased patch.

Comment on attachment 155510 [details] [review]
Don't destroy and recreate workspaces on view switches

(I'd already reviewed the previous version before you attached the rebased one...)

Much better.

>+        // Add workspace actor
>+        for (let w = 0; w < global.screen.n_workspaces; w++)

"actors", not "actor"

>+            let removedIndex = this.workspacesView.removedIndex;
>+            lostWorkspaces = this._workspaces.splice(removedIndex,
>+                                                     removedNum);

Meh, ok, this code was already somewhat broken before (in the linear view), but it's worse now. The problem is that it assumes that this code is only going to get run when someone clicks the [-] button (and so workspacesView.removeIndex has been initialized properly), but in fact, any call to global.screen.remove_workspace() anywhere in the shell when the overview is open will cause it to be invoked, and in that case, removedIndex would be junk. (It's even possible for multiple workspaces to be deleted at once.)

This shouldn't be hard to fix though. If you assume workspaces can't be added to the middle of the list (which I'm pretty certain is true), then all you have to do is walk this._workspaces, and if this._workspaces[i].metaWorkspace != global.screen.get_workspace_by_index(i), then delete that element from _workspaces. (And adjust the workspaceNum of any workspaces after the first deleted one.)

>+            // Don't let the user try to select this workspace as it's
>+            // making its exit.
>+            for (let l = 0; l < lostWorkspaces.length; l++)
>+                lostWorkspaces[l]._desktop.actor.reactive = false;

"_desktop" is a private variable. Either make it public, or add "Workspace.setReactive()" or something

Likewise with _metaWorkspace if you need to use that for the workspace-syncing above. It might also make sense to have the controller pass the metaWorkspace to the Workspace constructor rather than only passing the workspaceNum? (Actually, you could probably make Workspace work entirely in terms of metaWorkspace, not workspaceNum, and then you wouldn't need to worry about adjusting the workspaceNums when workspaces were deleted. In some places this would just mean using a method that takes/returns a workspace instead of a workspace index, and in others it would mean using this.metaWorkspace.get_index() instead of this.workspaceNum.)

>+            // reassign workspaceNum and metaWorkspace, as lost workspaces
>+            // have not necessarily been removed from the end
>+            for (let i = removedIndex; i < this._workspaces.length; i++) {
>+                let metaWorkspace = global.screen.get_workspace_by_index(i);
>+                this._workspaces[i].workspaceNum = i;
>+                this._workspaces[i]._metaWorkspace = metaWorkspace;
>+            }

Note that if this works, it means the last line is a no-op, since you're not regenerating this._workspaces[i]._windows, etc, so if the _metaWorkspace actually had changed, the object would be completely inconsistent as a result.

Comment on attachment 156293 [details] [review]
Don't destroy and recreate workspaces on view switches

see previous review. the drag-related bits look right

Comment on attachment 156294 [details] [review]
Animate workspace view switches

>+            originX = this._x + scale * dx

missing ";"

>+            workspace.positionWindows(Workspace.WindowPositionFlags.ANIMATE);

Why do we need to call that here? The windows shouldn't move during the transition.

>+            workspace._hideAllOverlays();

calling a private method. Either make it public or make that happen as a side-effect of one of the other calls, as appropriate.

>+            Tweener.addTween(workspace.actor,
>+                { x: workspace.gridX,

I guess the indentation was wrong before? The normal addTween indentation is

              Tweener.addTween(workspace.actor,
                               { x: workspace.gridX,

(basically all the same issues apply in the other updateWorkspaces method too)

otherwise looks good

Created attachment 156305 [details] [review]
Don't destroy and recreate workspaces on view switches

(In reply to comment #12)
> It might also make sense to have the controller pass the metaWorkspace
> to the Workspace constructor rather than only passing the workspaceNum?
> (Actually, you could probably make Workspace work entirely in terms of
> metaWorkspace, not workspaceNum

Sounds reasonable.


> >+                this._workspaces[i].workspaceNum = i;
> >+                this._workspaces[i]._metaWorkspace = metaWorkspace;
> >+            }
> 
> Note that if this works, it means the last line is a no-op

Indeed, it is a no-op - as workspaceNum is now gone, the whole loop is obsolete, hooray!

Created attachment 156307 [details] [review]
Animate workspace view switches

(In reply to comment #14)
> >+         workspace.positionWindows(Workspace.WindowPositionFlags.ANIMATE);
> 
> Why do we need to call that here? The windows shouldn't move during the
> transition.

I'm not sure if you are referring to positionWindows() or to the ANIMATE flag here:

positionWindows() is necessary because the new size/position is not exactly the same as the scaled old size/position (this is because the window caption and close button do not change size, but take part in the window sizing/positioning calculations). The function will also update the overlay positions.

As the new window position is slightly changed (relative to the workspace), omitting the ANIMATE flag would update the position immediately - it's a difference of few millimeters, but it's nevertheless noticable if not animated.

Created attachment 156460 [details] [review]
Don't destroy and recreate workspaces on view switches

Rebased patch.

Created attachment 156461 [details] [review]
Animate workspace view switches

Rebased patch.

Comment on attachment 156460 [details] [review]
Don't destroy and recreate workspaces on view switches

>-        Main.activateWindow(clone.metaWindow, time, this.workspaceNum);
>+        Main.activateWindow(clone.metaWindow, time,
>+                            this.metaWorkspace.index());

we should probably change Main.activateWindow to take a workspace instead of an index too (later, in another patch).

>+            // Assume workspaces are only removed sequentially
>+            // (e.g. 2,3,4 - not 2,4,7)
>+            let removedIndex;
>+            let removedNum = oldNumWorkspaces - newNumWorkspaces;
>+            for (let w = 0; w < oldNumWorkspaces; w++) {
>+                let metaWorkspace = global.screen.get_workspace_by_index(w);
>+                if (this._workspaces[w].metaWorkspace != metaWorkspace) {
>+                    removedIndex = w;
>+                    break;

this still only handles a single removed workspace. If you change the number of workspaces via gconf, then it can go from 4 to 1 in a single _updateWorkspaces call.

(Maybe we should fix it so we're not listening to the gconf num_workspaces key... but for now, we are.)

Comment on attachment 156461 [details] [review]
Animate workspace view switches

looks good to commit after the other patch is ready

(In reply to comment #19)
> >+            // Assume workspaces are only removed sequentially
> >+            // (e.g. 2,3,4 - not 2,4,7)
> >+            let removedIndex;
> >+            let removedNum = oldNumWorkspaces - newNumWorkspaces;
> >+            for (let w = 0; w < oldNumWorkspaces; w++) {
> >+                let metaWorkspace = global.screen.get_workspace_by_index(w);
> >+                if (this._workspaces[w].metaWorkspace != metaWorkspace) {
> >+                    removedIndex = w;
> >+                    break;
> 
> this still only handles a single removed workspace. If you change the number of
> workspaces via gconf, then it can go from 4 to 1 in a single _updateWorkspaces
> call.

In that case, removedIndex will be 1 and removedNum will be 3, so when calling this._workspaces.splice(1, 3) the last three workspaces are removed. In fact, I now tested the gconf case and it appears to work just fine (the removal animation regressed recently, but I don't think that's in the scope of this patch).

Comment on attachment 156460 [details] [review]
Don't destroy and recreate workspaces on view switches

duh, wasn't reading carefully

Attachment 156460 [details] pushed as a9fea82 - Don't destroy and recreate workspaces on view switches
Attachment 156461 [details] pushed as 5a6c9f1 - Animate workspace view switches

Created attachment 156527 [details] [review]
If current workspace changed during dnd, manually call _onWindowDragEnd.

Fix regression in Attachment 156460 [details]

Review of attachment 156527 [details] [review]:

Seems to work in a quick test, but I'd really appreciate some more comments of what's going on. It's not obvious from the code why you manually reset the drag in some places and not in others ...

::: js/ui/workspacesView.js
@@ +937,3 @@
 
+            this._onWindowDragEnd();
+        if (this._dragBeginOnWorkspace != global.screen.get_active_workspace_index())

_dragBeginOnWorkspace should be initialized to -1 or something in _init

@@ +967,3 @@
+                this._onWindowDragEnd();
+            if (this._dragBeginOnWorkspace == global.screen.get_active_workspace_index())
+        if (x > dx && y > dy && y < dy + dh) {

I suppose this is fine, but I'd really miss some comments in this function - e.g. why is it necessary to (eventually) cancel the drag when dropping on the (+) area, but not for the other drop targets?

@@ +1053,2 @@
         this._dropGroup.lower_bottom();
         }

Might be a good idea to reset _dragBeginOnWorkspace here.

Created attachment 156602 [details] [review]
Fix regressions of Dragging and dropping windows.

1. Update visibility of controls on view change.
2. [SingleView] Listen 'window-drag-begin'/'window-drag-end' signals on
all workspaces
3. [MosaicView] Fix dropping window on +.

Should we make the workspace that the item was dropped on active in all cases? We do that if the workspaces have scrolled through to fully show the workspace the item is being dropped on or if we are creating a new workspace, but not if you are dropping the item on the edge of the existing workspace.

Also, is it by design that we animate the window from it's original position instead of the preview that you are dragging when the mouse is released for a drop?

(In reply to comment #27)
> Should we make the workspace that the item was dropped on active in all cases?
> We do that if the workspaces have scrolled through to fully show the workspace
> the item is being dropped on or if we are creating a new workspace, but not if
> you are dropping the item on the edge of the existing workspace.

The current "mail slot" behavior for dropping on an existing workspace certainly surprised.
 
> Also, is it by design that we animate the window from it's original position
> instead of the preview that you are dragging when the mouse is released for a
> drop?

That's bug 613367.

There are some other regressions since the windows dnd:

 - captions fade in while workspaces still animate on scrollbar 
   snapping / indicator clicks

 - broken workspace removal animation

I'd like to see some cleanup similar to bug 609673 (cleaner separation between updating object parameters, setting up scrolling, updating actors, ...).

Some time ago, Dan proposed the addition of St.Group, which would be a nice replacement for WorkspacesView.actor and WorkspacesView._actor (ugggh).

Maybe it is a good idea to extend the feature freeze for workspace/workspacesView beyond the 2.29.1 release to do some major code cleanup ...

Created attachment 156656 [details] [review]
Fix regressions of Dragging and dropping windows

1. Update visibility of controls on view change.
2. [SingleView] Listen 'window-drag-begin'/'window-drag-end' signals on
all workspaces
3. [MosaicView] Fix dropping window on +.
4. fix captions fade in while workspaces still animate on scrollbar
   snapping / indicator clicks
5. fix broken workspace removal animation

Review of attachment 156656 [details] [review]:

This patch really should be broken up into a patch for problem, it's not at all easy for me to figure out.

Created attachment 156779 [details] [review]
add a missing semicolon

Created attachment 156780 [details] [review]
workspacesView.js: Fix dropping window on + in MosaicView.

Created attachment 156781 [details] [review]
workspacesView.js: listen to drag signals on all workspaces

Created attachment 156782 [details] [review]
workspacesView.js: fix broken workspace removal animation

Created attachment 156784 [details] [review]
workspace.js: fix caption fade in

captions were fading in while workspaces were still animating on
scrollbar snapping / indicator clicks

Created attachment 156785 [details] [review]
workspacesView.js: Update visibility of controls on view change.

Created attachment 156786 [details] [review]
???

(In reply to comment #38)
> Created an attachment (id=156786) [details] [review]
> ???

Fix dropping window on + after workspacesView switch.

Comment on attachment 156781 [details] [review]
workspacesView.js: listen to drag signals on all workspaces

>@@ -1490,6 +1488,10 @@ WorkspacesManager.prototype = {
>         else
>             viewType = WorkspacesViewType.SINGLE;
> 
>+        for (let w = 0; w < this._workspaces.length; w++) {
>+            this._workspaces[w].disconnectAll();
>+        }

It's sorta plausible to use disconnectAll in the case where we're destroying the workspace object immediately afterward, but using it here when switching between views seems too fragile; there could easily end up being some other signal connection in the future that we *don't* want to disconnect from at this point.

For that matter, it's dubious for the WorkspacesManager to be doing the disconnecting anyway, since it's not the one who made the connections; if the views make the connections, they should disconnect them themselves as well.

Comment on attachment 156782 [details] [review]
workspacesView.js: fix broken workspace removal animation

>Subject: [PATCH] workspacesView.js: fix broken workspace removal animation

what's broken? I'm not seeing it.

Comment on attachment 156784 [details] [review]
workspace.js: fix caption fade in

not crazy about this, since it depends on the fact that someone else calls this._windowOverlaysGroup.show() in all relevant cases... but that's part of the general "overview needs refactoring" thing I guess

Comment on attachment 156785 [details] [review]
workspacesView.js: Update visibility of controls on view change.

>         this.controlsBar.updateControls(this.workspacesView);
> 
>+        this.controlsBar.setCanAdd(this.workspacesView.canAddWorkspace());
>+        this.controlsBar.setCanRemove(this.workspacesView.canRemoveWorkspace());

seems like maybe updateControls() should do that itself?

Comment on attachment 156786 [details] [review]
???

> Fix dropping window on + after workspacesView switch.

ah, ok. feel free to re-merge this with the other dropping-on-+ patch

Comment on attachment 156782 [details] [review]
workspacesView.js: fix broken workspace removal animation

clarified on IRC: the disappearing workspace currently zooms away from the other workspaces too quickly.

>+            for (let i = 0; i < lostWorkspaces.length; i++)
>+                lostWorkspaces[i].index = removedIndex + i;

this seems wrong. If we need to know the workspace indexes after they're deleted, then let's just put back workspaceNum.

as with one of the other patches, I dont like this one, but we can hopefully clean things up later. The way that the linear view reuses random bits of the grid view architecture (eg, the gridX, gridY, etc, members) for not-quite-related things is just a mess.

Created attachment 156793 [details] [review]
workspacesView.js: listen to drag signals on all workspaces

(In reply to comment #45)
> this seems wrong. If we need to know the workspace indexes after they're
> deleted, then let's just put back workspaceNum.

I agree that this seems wrong, but I don't think we need to bring back workspaceNum to fix it. I'd like to try to separate the workspace switching from the workspace zooming (when dragging a window) - I'm slightly optimistic that this fixes the issue as well.

So can we leave this patch out for the time being?

Created attachment 156815 [details] [review]
workspacesView.js: Update visibility of controls on view change.

Review of attachment 156815 [details] [review]:

Looks good - two minor comments:

::: js/ui/workspacesView.js
@@ +1419,3 @@
         this._gconf.set_string(WORKSPACES_VIEW_KEY, view);
         this._currentViewType = view;
+        this.updateControlsSensitivity();

Why is this necessary? When the gconf key changes, updateControlsSensitivity() is called twice (in updateControls() and in _setView())
I tried removing the call in _setView() without noting a difference.

@@ +1423,2 @@
     },
+    _setCanRemove: function(canRemove) {

Now that you're updating the button sensitivity in WorkspacesControls itself, there is no real reason in keeping this in a separate function (same applies for _setCanAdd) - just call _setButtonSensitivity() from updateControlsSensitivity()

Created attachment 156822 [details] [review]
[linearView] Separate workspace scrolling and zooming

The zoom effect when dragging a window was done together with the
workspace scrolling - as those animations are completely different,
they should be handled separately.
With the split, two recent regressions are fixed: captions are no
longer faded in while still scrolling, and correct spacing is used
in the workspace removal animation.

Sorry for the large patch, most of it just shifts code around - the scrolling part is reverted more or less to the split which existed before the window dnd landed, the remaining code is moved into separate functions.

(I don't mark attachment 156782 [details] [review] and attachment 156784 [details] [review] as obsolete for now, although the patch fixes those issues)

Created attachment 156824 [details] [review]
workspacesView.js: Update visibility of controls on view change.

Review of attachment 156822 [details] [review]:

The cleanups in detail here look great. I'm not really sold on the idea of splitting zooming and scrolling - that seems to me to cause problems if they ever overlap. I'd like to see the code that hides and shows actors more centralized rather than scattered over the code as piecemeal updates.

::: js/ui/workspacesView.js
@@ +549,3 @@
     },
 
     _positionWorkspaces: function() {

Think this would be better as _computeWorkspacePositions, and/or maybe just needs a comment along the lines of 

  // Computes the position and scale of the workspaces, but doesn't actually
  // change the actors to match

@@ +562,3 @@
+
+        this._offsetX = (this._width - _width) / 2;
+        this._offsetY = (this._height - _height) / 2;

It's not obvious that a member variable called 'offsetX' is X position of the active workspace actor with respect to workspaces area; if that is the correct understand you should do one or both of a) rename it to activeWorkspaceX b) add a comment, probably in the _init function.

@@ +567,3 @@
             let workspace = this._workspaces[w];
 
+            workspace.opacity = (this._inDrag && active != w ) ? 200 : 255;

Very minor point, but more readable as 'w != active' since what we are asking is "is w the active workspace?"

@@ +596,3 @@
+    },
+
+    _updateWorkspaceActors: function() {

A one-two line comment about what this function does and when it should be called would be useful; the name is very generic.

@@ -789,4 +803,2 @@
                 Tweener.addTween(workspace.actor,
                     { x: workspace.gridX,
-                      y: workspace.gridY,
-                      scale_x: workspace.scale,

Hmm, so what you are hoping here is that if there is some previous zoom-in/zoom-out tween, then it will keep on tweening the scale, but we override the X position tween with this tween?

I'm not sure I like multiple overlapping tweens going on for the same actor - the whole approach of splitting the zoom and the scroll apart seems maybe a little suspect to me - unless we actually enforce that they can't overlap. It seems more robust to always compute a "target state", show all actors that are visible in the target state, hide overlays we don't want to animate, tween to the target state, and then when we're done with all animations hide actors that aren't visible, and show overlays.

@@ +809,3 @@
+                              if (visible) {
+                                  if (!this._inDrag)
+                                      workspace.showWindowsOverlays();

Hmm, I'm pretty confused by a 'visible' variable that is set outside the closure depending on this._inDrag and then referenced inside closure with another conditional based on this._inDrag at another point in time.

@@ +811,3 @@
+                                      workspace.showWindowsOverlays();
+                              } else {
+                                  workspace.actor.hide();

What if this._inDrag got changed since visible was defined?

I'd rather see a single function that updated workspace state (hidden, overviews) based on the current state of the overview - are we dragging, are we animating, what is the current workspace.

@@ +819,2 @@
                     if (!this._inDrag)
                         workspace.showWindowsOverlays();

You've duplicated a rather complicated conditional from above but with a somewhat complicated interaction based on time.

@@ +853,3 @@
+                               onComplete: function() {
+                                   if (this._inDrag)
+                                       this._updateShadowVisibility();

Hmm, not completely sure if it's right to wait until the end to update shadow visibility, but as long as it doesn't *look* bad to have the shadow abruptly appear at the end when things are running at full speed, there's no reason to add complexity.

This might belong in a centralized state-updating function.

@@ +891,3 @@
+
+            workspace.actor.show();
+            workspace.hideWindowsOverlays();

What shows the window overlays again? Again, we have a lot of different code showing and hiding things and it seems likely that there are sequences where things go wrong. I'd rather have it be (schematically);

 this._zoomingWorkspaces = true;
 this._updateVisibility();
 Tweener.addTween(..., onComplete: .. 
     this._zoomingWorkspaces = false;
     this._updateVisibility();
 }

(In reply to comment #52)
> I'm not sure I like multiple overlapping tweens going on for the same actor
[snip]
> I'd rather have it be (schematically);
> 
>  this._zoomingWorkspaces = true;
>  this._updateVisibility();
>  Tweener.addTween(..., onComplete: .. 
>      this._zoomingWorkspaces = false;
>      this._updateVisibility();
>  }

Mmmh - those comments conflict?

(In reply to comment #53)
> (In reply to comment #52)
> > I'm not sure I like multiple overlapping tweens going on for the same actor
> [snip]
> > I'd rather have it be (schematically);
> > 
> >  this._zoomingWorkspaces = true;
> >  this._updateVisibility();
> >  Tweener.addTween(..., onComplete: .. 
> >      this._zoomingWorkspaces = false;
> >      this._updateVisibility();
> >  }
> 
> Mmmh - those comments conflict?

I don't really see that - I meant that you'd Tweener.removeAllTweens() for the tweened actors and then add new tweens.

Created attachment 157021 [details] [review]
[linearView] Cleanup workspace scrolling and zooming

Centralize the update of actor visibilities (overlays, shadows,
off-screen workspaces). Adjust the way scrolling is handled so
that it works correctly with removed workspaces.

(In reply to comment #54)
> > (In reply to comment #52)
> > >  this._zoomingWorkspaces = true;
> > >  this._updateVisibility();
> > >  Tweener.addTween(..., onComplete: .. 
> > >      this._zoomingWorkspaces = false;
> > >      this._updateVisibility();
> > >  }
> I meant that you'd Tweener.removeAllTweens() for the tweened actors and
> then add new tweens.

OK, didn't quite get that. There's a problem with the proposal though: the onComplete handler is not called on Tweener.removeTweens(), so if we control different state variables there we risk ending up in an unconsistent state.

To avoid this I dropped the separation - the patch does now:

 - change the way scrolling is done in _updateWorkspaceActors() to work with
   removed workspaces as well

 - add _updateVisibility() which handles showing/hiding actors centrally

 - add an additional state variable (SingleView._animating) which is set
   to %true during tweens

 - show overlays when in window-drag mode, except for the dragged window
   (this is different than the current behavior - it's consistent with
    grid mode, so it seemed like a good idea; easy to drop if unwanted)

Of course, comments which still apply have been addressed ...

Review of attachment 156824 [details] [review]:

Looks good to me.

Review of attachment 156824 [details] [review]:

Looks good to me.

Review of attachment 157021 [details] [review]:

I really like how this reads, and I couldn't find anything to comment on reading the pach. There is an observed problem:

> - show overlays when in window-drag mode, except for the dragged window
>   (this is different than the current behavior - it's consistent with
>    grid mode, so it seemed like a good idea; easy to drop if unwanted)

The problem here is that the overlays are hidden as we zoom out, then reshown. If this is going to be really hard to fix, then maybe we should just ignore it - it's only really noticeable when you are actually looking at a window other than the one you are dragging (or maybe if you have huge numbers of windows on the workspace.)

Review of attachment 157021 [details] [review]:

I really like how this reads, and I couldn't find anything to comment on reading the pach. There is an observed problem:

> - show overlays when in window-drag mode, except for the dragged window
>   (this is different than the current behavior - it's consistent with
>    grid mode, so it seemed like a good idea; easy to drop if unwanted)

The problem here is that the overlays are hidden as we zoom out, then reshown. If this is going to be really hard to fix, then maybe we should just ignore it - it's only really noticeable when you are actually looking at a window other than the one you are dragging (or maybe if you have huge numbers of windows on the workspace.)

(In reply to comment #59)
> The problem here is that the overlays are hidden as we zoom out, then reshown.
> If this is going to be really hard to fix, then maybe we should just ignore it

Depends on the fix:
1. Keeping overlays hidden while in drag mode == trivial
2. Keeping overlays around while zooming into drag mode == hard

2. would imply adding support for animations to the overlays - we could then get rid of the hiding in other places as well, e.g. when adding workspaces in grid view; sounds like a good idea to me, but I'd rather leave that as a future improvement.

(In reply to comment #60)
> (In reply to comment #59)
> > The problem here is that the overlays are hidden as we zoom out, then reshown.
> > If this is going to be really hard to fix, then maybe we should just ignore it
> 
> Depends on the fix:
> 1. Keeping overlays hidden while in drag mode == trivial
> 2. Keeping overlays around while zooming into drag mode == hard
> 
> 2. would imply adding support for animations to the overlays - we could then
> get rid of the hiding in other places as well, e.g. when adding workspaces in
> grid view; sounds like a good idea to me, but I'd rather leave that as a future
> improvement.

Yeah, let's get this in. I'll leave it up to you whether you want to do 1. or nothing for now.

Attachment 157021 [details] pushed as 082a15b - [linearView] Cleanup workspace scrolling and zooming