Created attachment 153565 [details] [review]
[Workspaces] Clean up scrolling in linear view

Reorganize the code to break up positioning into:

1) scaling and lining up of the workspaces
2) scrolling the view to a particular workspace
3) handling dragging of the scroll bar

With these cleanups, it becomes much easier to fix
the following issues:

- use animations consistantly instead of doing hard breaks
  for some actions and smooth transitions for others
- snap to the closest workspace when scrolling stops
  (https://bugzilla.gnome.org/show_bug.cgi?id=607823)
- fix the regression of the zoomFromOverlay animation when
  the selected app is on another workspace
  (https://bugzilla.gnome.org/show_bug.cgi?id=609081)

Created attachment 153566 [details] [review]
Make spacing between workspaces stylable

Currently the width of the gaps between workspaces in both linear
and mosaic view are defined as constants. Move these to the theme's
CSS instead.

Created attachment 153567 [details] [review]
Slide out removed workspaces in linear view

Instead of deleting workspaces immediately, animate the removal - it may
be useless eye-candy, but it's pretty sexy nonetheless ...
More seriously, the animation improves consistency with both workspace
additions and the mosaic view.

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

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

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

Looks nice from my quick testing.

Created attachment 153675 [details] [review]
[Workspaces] Clean up scrolling in linear view

Minor improvement to the scroll bar animation after adding a new workspace.

Created attachment 153676 [details] [review]
Slide out removed workspaces in linear view

Rebased to work with the patch in attachment 153675 [details] [review]

Created attachment 153854 [details] [review]
[Workspaces] Clean up scrolling in linear view

The method _positionWorkspaces() works slightly different in MosaicView
and SingleView: in the former, only the workspace object's attributes are
updated, while the latter also adjust the workspace's actor.

Updated the method in SingleView to behave like the one in MosaicView.

Created attachment 153855 [details] [review]
Make spacing between workspaces stylable

Rebase to master

Created attachment 153856 [details] [review]
Slide out removed workspaces in linear view

Rebased on top of the first patch.

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

Review of attachment 153854 [details] [review]:

Looks really good. My comments here are all stylistic - feel free to commit when you've fixed these up.

::: js/ui/workspacesView.js
@@ +500,1 @@
+        for (let w in this._workspaces) {

Remember that iterating over an array this way isn't guaranteed to iterate in order (weirdly!), I think it's confusing to have one way to iterate over an array in order, and another way when we don't care about order, so I think it would be better to just stick to w++ style iteration over arrays.

@@ +532,3 @@
+    _scrollWorkspacesToIndex: function(index, showAnimation) {
+        let active = global.screen.get_active_workspace_index();
+        let fx = this._x - this._workspaces[index].gridX;

This expression was very obscure to me. Spelling it out as, say:

let current_active_workspace_position = this._workspaces[index].gridX;
let new_active_workspace_position = this._x;
let dx = new_active_workspace_position - current_active_workspace_position;

(dx not fx to stand for Δx), suddenly makes it make sense.

@@ +569,3 @@
+        if (this._onScrollId > 0) {
+            this._scroll.adjustment.disconnect(this._onScrollId);
+            this._onScrollId = 0;

I think it's usually cleaner and simpler to use a flag rather than disconnecting and reconnecting signal connections. To simple say this._animatingScroll = true, and if (this._animatingScroll) return;

@@ +664,3 @@
+
+        let fx;
+        let n = this._workspaces.length - 1;

A variable called n should obviously be the number of something. I guess this could be the be the "number of gaps", but it's better, I think to just use n as this._workspaces.length and write '/ (n - 1)'

@@ +668,3 @@
+            // The scrollbar is hidden when there is only one workspace, so
+            // n should never be 0 - but assure we don't divide by zero anyway.
+            fx = this._x - this._workspaces[0].actor.x;

Would it be better to just return if you want to be defensive? It couldn't work out if fx turned out to be anything than strictly zero, right?

@@ +671,3 @@
+        else {
+            let w = this._workspaces[n].actor.x - this._workspaces[0].actor.x;
+            fx = this._x - adj.value * w / n - this._workspaces[0].actor.x;

Even more than the other place, this doesn't read to me as making sense. I think I can reverse engineer it as;

 let total_width = this._workspaces[n].actor.x - this._workspaces[0].actor.x;
 let current_position = this._workspaces[0].actor.x;
 let new_position = this._x - adj.value * total_width / n;
 let dx = new_position - current_position;

but then it should be written that way, not simplified into something less obvious. (Maybe it's obvious to you, it's not obvious to me :-)

I'd use dx (as a stand-in for Δx) rather than fx - I'm not sure what fx is supposed to stand for.

Review of attachment 153855 [details] [review]:

Looks good.

::: data/theme/gnome-shell.css
@@ +163,3 @@
 }
 
+.workspaces.single {

You know, it never occurred to me to use multiple class selectors like this... I've typically done

 style_class: 'workspaces workspaces-single'

The obvious caveat to this pattern of using a "modifier selector" is that we never the modifier shouldn't be a reasonable main style class. If the modifier was a reasonable style class by itself, then a rule like '.single { background: red }' in some unrelated portion of the stylesheet would mess things up. But other than that, I can't think any of any problems with the pattern.

Review of attachment 153856 [details] [review]:

I certainly like animating back to the previous workspace better than just abruptly changing. I don't find the new cleanWorkspaces function at all readable, however, so it's hard to tell if it's correct or not.

::: js/ui/workspacesView.js
@@ +589,3 @@
+    },
+
+    _cleanWorkspaces: function() {

Your code mostly doesn't make things any worse, just moves things into a function, but this function is a mess of doing weird things without any comments. Why does removing a workspace require destroying all the other workspaces and recreating them? Without understanding that, it's hard, for example, to understand why is it OK to temporarily not recreate the workspaces and only do after the animation finishes.

@@ +598,3 @@
+
+        let active = global.screen.get_active_workspace_index();
+        let n_workspaces = global.screen.n_workspaces;

You don't use either of these variables?

@@ +604,3 @@
+            this._workspaces[i].destroy();
+        this._workspaces.splice(0);
+        this._actor.remove_all();

Didn't we just destroy everything?

@@ +606,3 @@
+        this._actor.remove_all();
+
+        // Without this there will be lots of warnings

What sort of warnings? Why?

@@ +607,3 @@
+
+        // Without this there will be lots of warnings
+        this._actor.hide();

There are two calls to this._actor().hide here.

Created attachment 154028 [details] [review]
Slide out removed workspaces in linear view

(In reply to comment #16)
> Review of attachment 153856 [details] [review]:
> I certainly like animating back to the previous workspace better than just
> abruptly changing. I don't find the new cleanWorkspaces function at all
> readable, however, so it's hard to tell if it's correct or not.

Yeah, you're right ... patch with a hopefully better rewrite of cleanWorkspaces().

Review of attachment 154028 [details] [review]:

Much more readable! Would it be a good idea to reassign the workspaceNum and _metaWorkspace before the animation rather than after? If that isn't feasible, I think it's OK this way - my only worry would be that, say, if the user clicked while the animation was in progress we'd try to switch to a workspaceNum that didn't exist, or some problem along those lines.

(In reply to comment #18)
> Review of attachment 154028 [details] [review]:
> Much more readable! Would it be a good idea to reassign the workspaceNum and
> _metaWorkspace before the animation rather than after?

Maybe - I'll give it a shot.

Comment on attachment 153855 [details] [review]
Make spacing between workspaces stylable

Attachment 153855 [details] pushed as b84a704 - Make spacing between workspaces stylable

Attachment 154028 [details] pushed as 3c8ba34 - Slide out removed workspaces in linear view
Attachment 153854 [details] pushed as 8ef7552 - Clean up scrolling in linear view