Created attachment 163030 [details] [review]
[linearView] Update workspace opacity

While zoomed out in drag mode, the workspaces at the left and right
of the active workspace are slightly transparent - when switching
workspaces, one of the transparent workspaces becomes the new active
workspace, but its opacity is not updated.

Review of attachment 163030 [details] [review]:

Generally confused by the existing code...

_updateWorkspaceActors will have done workspace.actor.opacity = workspace.opacity, so it appears that workspace.opacity has gotten out of sync with the opacity that the workspace "should have" - and it doesn't seem right to me to set workspace.actor.opacity  workspace.opacity then correct it back to what it "should have".... workspace.opacity should be the "should have value", and indeed computeWorkspacePosition has 'workspace.opacity = (this._inDrag && w != active) ? 200 : 255' - so being the active workspace during a drag seems to be expected to be accounted for in workspace.opacity.

The manual update of workspace.actor.opacity without changing workspace.opacity in handleDragOver() looks suspicious to me.

Created attachment 163390 [details] [review]
[linearView] Update workspace opacity

(In reply to comment #2)
> [...] indeed computeWorkspacePosition has 'workspace.opacity = (this._inDrag && w
> != active) ? 200 : 255' - so being the active workspace during a drag seems to
> be expected to be accounted for in workspace.opacity.

Kind of - computeWorkspacePosition() is not called on workspace switches. This patch changes that, though I'm not too happy to do a bunch of unnecessary calculations (workspace position relative to each other) in order to update a single property. Well, I guess we are doing more expensive stuff elsewhere ...


> The manual update of workspace.actor.opacity without changing workspace.opacity
> in handleDragOver() looks suspicious to me.

Fixed. As handleDragOver() is going away with the patches in bug 620504 with the replacement doing the same, there is now a dependency on said bug ...

(In reply to comment #3)
> Created an attachment (id=163390) [details] [review]
> [linearView] Update workspace opacity
> 
> (In reply to comment #2)
> > [...] indeed computeWorkspacePosition has 'workspace.opacity = (this._inDrag && w
> > != active) ? 200 : 255' - so being the active workspace during a drag seems to
> > be expected to be accounted for in workspace.opacity.
> 
> Kind of - computeWorkspacePosition() is not called on workspace switches. This
> patch changes that, though I'm not too happy to do a bunch of unnecessary
> calculations (workspace position relative to each other) in order to update a
> single property. Well, I guess we are doing more expensive stuff elsewhere ...

I wasn't saying that you needed to computeWorkspacePosition() on workspaces switches, but rather that things had to be updated *as if* it had been called; that is, that workspace.opacity should always reflect the desired opacity of the workspace.

On the other hand, computeWorkspacePositions() doesn't seem to be doing anything genuinely expensive, and it's certainly simplest to keep the logic in one place, as this patch does.

Review of attachment 163390 [details] [review]:

Very simple. Simple is good. Also seems to work right for me in testing.

Attachment 163390 [details] got pushed as d8f7629a4 ...