Created attachment 154554 [details] [review]
[Overview] Switch workspace when dragging the desktop

Depending on screen size and pointer position, the scroll bar used
to switch the active workspace in linear view may not be a convenient
target, so allow dragging the desktop as an alternative.

(In reply to comment #0)
> Some days ago there was some IRC discussion about allowing to switch workspaces
> with drag and swipe of the desktop - IMHO that's an interestng alternative to
> using the scrollbar, and a nice complement to bug 607821.

(In reply to comment #1)
> Created an attachment (id=154554) [details] [review]
> [Overview] Switch workspace when dragging the desktop
> 
> Depending on screen size and pointer position, the scroll bar used
> to switch the active workspace in linear view may not be a convenient
> target, so allow dragging the desktop as an alternative.

This works better than expected (didn't like the idea because it seemed odd to me), but after actually trying it I like it ;)

Switching this way feels way more natural than using the scrollbar or clicking on the indicators. (Feels like a big iPhone ;) )

This needs an ui-review though but Jon wasn't against this idea (he was in favor of at least trying it).

This complements some of the work in bug 607821.  I think it is an interesting thing to try.

Review of attachment 154554 [details] [review]:

Looks good overall and as I said before works fine for me.

Just some minor (style) comments.

Jon can we read your comment as "yes we want to get it in" ?

::: js/ui/workspacesView.js
@@ +691,3 @@
+        switch (event.type()) {
+
+        case Clutter.EventType.BUTTON_RELEASE:

Missing indention ... should be like

switch (foo) {
   case bar:
       ....

@@ +732,3 @@
+            return true;
+
+        case Clutter.EventType.MOTION:

Same here

@@ +738,3 @@
+
+            this._scroll.adjustment.value += (dx / primary.width);
+            this._dragDirection = - dx;

Remove the space it looks odd (i.e just write -dx)

(In reply to comment #4)
> ::: js/ui/workspacesView.js
> @@ +691,3 @@
> +        switch (event.type()) {
> +
> +        case Clutter.EventType.BUTTON_RELEASE:
> 
> Missing indention ... should be like

Dammit! I always indent the case line, but as there's quite a diversity of styles here I grepped for it to match the overall style - of course I had to pick the _only_ one which does not indent (calendar.js).

So yeah, I'll be more than happy to fix that ...

Created attachment 154621 [details] [review]
[Overview] Switch workspace when dragging the desktop

Fix style according to review.

Review of attachment 154621 [details] [review]:

Looks good I'd say get an ack from Jon and commit (unless someone else finds flaws in the code ;) )

Review of attachment 154621 [details] [review]:

Looks pretty good to me.

Created attachment 155053 [details] [review]
[Overview] Switch workspace when dragging the desktop

Correct the bounds check of the workspace index in _onCapturedEvent() to
apply to the new index rather than the currently active one - this fixes
the issue that the workspace was snapped back when the handle was dragged
far enough to trigger a workspace switch before releasing the pointer.

Created attachment 155058 [details] [review]
[Overview] Switch workspace when dragging the desktop

Played around with Jon on IRC - it seems better to just remove the complicated rules for cancelling a switch: just do it according to the drag direction and be done :)

I'd like to wait on this patch until we land bug 607821

Created attachment 156292 [details] [review]
[Overview] Switch workspace when dragging the desktop

Rebased patch.

Review of attachment 156292 [details] [review]:

Works very well!  Two minor comments.

::: js/ui/workspacesView.js
@@ +596,3 @@
+    // @draggable: whether workspace @index should be draggable
+    // @index: workspace index
+    // _setWorkspaceDraggable:

I think I'd prefer this to be:

if (draggable)
  this._workspaces[index].actor.reactive = true;

Which has the property that it won't break if we later make the workspace actor always reactive (think a slight hover effect or something).

@@ +645,3 @@
+
+                [stageX, stageY] = event.get_coords();
+

This is fairly minor, but I think it's worth doing a bit of checking here to see if we've dragged more than say 20% of the way; if we haven't, just snap back.

Created attachment 156416 [details] [review]
[Overview] Switch workspace when dragging the desktop

> This is fairly minor, but I think it's worth doing a bit of checking here to
> see if we've dragged more than say 20% of the way; if we haven't, just snap
> back.

Not sure if I like it - it's no longer possible to just grab the desktop and throw it in the desired direction; but then, it might be better to implement that in another patch with a proper animation ...
The exact threshold is up to debate of course (patch goes with the proposed 20%).

Attachment 156416 [details] pushed as 47644ca - [Overview] Switch workspace when dragging the desktop