Created attachment 153190 [details] [review]
New workspace switcher popup

This patch implements this.

I am not sure about the icons, we probably need custom icons for this (I suck at creating icons, so this would be a task for someone else ;) )

Created attachment 153211 [details] [review]
New workspace switcher popup

New version based on the mockups from:

http://www.gnome.org/~mccann/screenshots/clips/20100207160938/

Created attachment 153214 [details] [review]
New workspace switcher popup

*) Use 96x96 icons to be closer to the alt-tab dialog

We iterated a bit on irc.  Looking pretty good.

I like that this is consistent with the size and position of the alt tab overlay.

The arrow is a good on-screen confirmation/feedback for the the hotkey that was used.  Similar to media eject or volume.

A few comments:
 * the space between workspaces is twice as wide as I'd like it.
 * I think we should try to dim down the current selected box slightly by adding more alpha probably.  It's just a little too in your face right now.
 * I am not sure I like the border on the selected box.  However, we should try to ensure the boxes are all the same size (eg. compensate for border width).
 * I tried doing some of these things with the css but things behaved very strangely.  When changing the alpha for the box borders for example changes seemed non-linear.  Some weird inheritance effects maybe?

 * I don't know if we need special behavior when the end is reached or not.

Created attachment 153225 [details] [review]
New workspace switcher popup

Fixed up patch.

*) better spacing
*) flipped the image rather than rotating it
*) code cleanups

Nice.  I think I like it with the following a bit better though:
index 6a6af7c..e76df41 100644
--- a/data/theme/gnome-shell.css
+++ b/data/theme/gnome-shell.css
@@ -636,21 +636,21 @@ StTooltip {
     background: rgba(0,0,0,0.8);
     border: 1px solid rgba(128,128,128,0.40);
     border-radius: 8px;
-    padding: 18px;
+    padding: 12px;
 }
 
 .workspace-switcher {
     background: transparent;
     border: 0px;
     border-radius: 0px;
-    padding: 8px;
+    padding: 4px;
 }
 
 .workspace-switcher .active-left {
     width: 98px;
     height: 98px;
     border: 0px;
-    background: rgba(255,255,255,0.8);
+    background: rgba(255,255,255,0.5);
     background-image: url("ws-switch-arrow-left.png");
     border-radius: 4px;
 }
@@ -659,7 +659,7 @@ StTooltip {
     width: 96px;
     height: 96px;
     border: 0px;
-    background: rgba(255,255,255,0.8);
+    background: rgba(255,255,255,0.5);
     background-image: url("ws-switch-arrow-right.png");
     border-radius: 4px;
 }

Created attachment 153227 [details] [review]
New workspace switcher popup

Added CSS changes from comment 6

Created attachment 153229 [details] [review]
New workspace switcher popup

*) Minor cleanup

Created attachment 153231 [details] [review]
New workspace switcher popup

Make sure that altTab and the workspaceSwicher never show up at once.

Created attachment 153291 [details] [review]
New workspace switcher popup

Rebased to current head.

Comment on attachment 153291 [details] [review]
New workspace switcher popup

OK, I'm assuming Jon is completely OK with the visual style so I'm not commenting on that at all.

>+        this._switcherPopup = null;

should specify that it's the *workspace* switcher. I'd call it "this._workspaceSwitcher".

>+    _startWorkspaceSwitcher : function(shellwm, binding, window, backwards) {

_startAppSwitcher is called that because that's all it's used for; it *starts* the app switcher, but then the switcher is entirely run out of altTab.js. But with the workspace switcher, each call to "_startWorkspaceSwitcher" is only handling a single keystroke, so the "start" in the name doesn't make sense. Call it "_switchWorkspace".

>+        /* We don't support this kind of layout */
>+        if (binding == "switch_to_workspace_up" || binding == "switch_to_workspace_down")
>+            return;

Yeah, and this is a problem; we have the grid layout by default in the overview, but the linear layout in the switcher.

>+        if (this._switcherPopup == null || this._switcherPopup.isFadding)
>+            this._switcherPopup = new WorkspaceSwitcherPopup.WorkspaceSwitcherPopup();

"Fading". one "d". Needs to be fixed everywhere. (including once in 

Nothing ever clears this._switcherPopup after the switcher goes away, so the old switcher doesn't get freed until you create a new one (which gets created because the old one still has "fading" set, even though it's fully faded). You should make it so you can just keep reusing the old one; if you call display() on it again either while it's fading or after it has faded, it should unfade/reshow itself.

>+            this._switcherPopup.display(WorkspaceSwitcherPopup.LEFT);

might as well pass the workspace number to display() as well, so it doesn't have to re-check it itself.

>+            let inidcator = null;

"indicator"

>+               let spacer = new St.Bin({ style_class: 'workspace-switcher spacer' });

it seems wacky to have the "workspace-switcher" class set on both the boxes and the spacers. "workspace-switcher" doesn't have any real meaning in that case, it's just "the subset of CSS attributes that we happen to want on both boxes and spacers".

>+        Mainloop.source_remove(this._timeoutId);

should "this._timeoutId = 0" too

>+        Tweener.addTween(this._container, { opacity: 0.0,
>+                                time: ANIMATION_TIME,
>+                                transition: "easeOutQuad",
>+                                onComplete: this.actor.hide(),
>+                                onCompleteScope: this

alignment is wrong. Also, you're *calling* this.actor.hide() rather than just referring to it, so the popup gets hidden before the animation happens. (And you ought to be passing "this.actor" for onCompleteScope in this case anyway. But just do:

    onComplete: function() { this.actor.hide() },
    onCompleteScope: this

it's clearer.)

(In reply to comment #11)
> >+        /* We don't support this kind of layout */
> >+        if (binding == "switch_to_workspace_up" || binding == "switch_to_workspace_down")
> >+            return;
> 
> Yeah, and this is a problem; we have the grid layout by default in the
> overview, but the linear layout in the switcher.

filling in more from IRC, there are currently 3 workspace layouts:

    - The overview (which defaults to oddly-numbered grid, but also does
      linear)

    - The switcher (which only does linear)

    - Metacity-internal (which defaults to linear, but can be tricked
      by third parties into being a normally-numbered grid (ie, not the
      same as the overview grid) which shows up in the default workspace
      switcher and also in the titlebar menu ("Move to workspace right",
      etc)

It would be good if they all agreed, either by there only being one supported layout, or by them all reflecting whichever layout you used last in the overview.

(Yeah, this will require some mutter hacking to fix the metacity-internal one, unless we pick "linear only" in which case we win. Mostly; you can still trick metacity by starting up a non-applet-based pager program, which could use EWMH hints to force mutter into a different layout, which would still be visible in the menus. So, mutter hacking either way.)

(In reply to comment #11)
> (From update of attachment 153291 [details] [review])
> OK, I'm assuming Jon is completely OK with the visual style so I'm not
> commenting on that at all.
> 
> >+        this._switcherPopup = null;
> 
> should specify that it's the *workspace* switcher. I'd call it
> "this._workspaceSwitcher".

Yeah that makes sense.

> >+    _startWorkspaceSwitcher : function(shellwm, binding, window, backwards) {
> 
> _startAppSwitcher is called that because that's all it's used for; it *starts*
> the app switcher, but then the switcher is entirely run out of altTab.js. But
> with the workspace switcher, each call to "_startWorkspaceSwitcher" is only
> handling a single keystroke, so the "start" in the name doesn't make sense.
> Call it "_switchWorkspace".

Well in that case I'd prefer a name like "_showWorkspaceSwitcher" ... we already have a "_switchWorkspace" in windowManager.js btw ;)

> >+        /* We don't support this kind of layout */
> >+        if (binding == "switch_to_workspace_up" || binding == "switch_to_workspace_down")
> >+            return;
> 
> Yeah, and this is a problem; we have the grid layout by default in the
> overview, but the linear layout in the switcher.

Yeah I am not sure what to do about this ... IRC discussion did not have an outcome yet.

> >+        if (this._switcherPopup == null || this._switcherPopup.isFadding)
> >+            this._switcherPopup = new WorkspaceSwitcherPopup.WorkspaceSwitcherPopup();
> 
> "Fading". one "d". Needs to be fixed everywhere. (including once in 

... ok ;) 

once in what?

> Nothing ever clears this._switcherPopup after the switcher goes away, so the
> old switcher doesn't get freed until you create a new one (which gets created
> because the old one still has "fading" set, even though it's fully faded). You
> should make it so you can just keep reusing the old one; if you call display()
> on it again either while it's fading or after it has faded, it should
> unfade/reshow itself.

OK

> >+            this._switcherPopup.display(WorkspaceSwitcherPopup.LEFT);
> 
> might as well pass the workspace number to display() as well, so it doesn't
> have to re-check it itself.

OK

> >+            let inidcator = null;
> 
> "indicator"

OK

> >+               let spacer = new St.Bin({ style_class: 'workspace-switcher spacer' });
> 
> it seems wacky to have the "workspace-switcher" class set on both the boxes and
> the spacers. "workspace-switcher" doesn't have any real meaning in that case,
> it's just "the subset of CSS attributes that we happen to want on both boxes
> and spacers".

OK, so I should use different css classes (i.e not referring to workspace-switcher at all) for the boxes and spacers?

> >+        Mainloop.source_remove(this._timeoutId);
> 
> should "this._timeoutId = 0" too

OK

> >+        Tweener.addTween(this._container, { opacity: 0.0,
> >+                                time: ANIMATION_TIME,
> >+                                transition: "easeOutQuad",
> >+                                onComplete: this.actor.hide(),
> >+                                onCompleteScope: this
> 
> alignment is wrong. Also, you're *calling* this.actor.hide() rather than just
> referring to it, so the popup gets hidden before the animation happens. (And
> you ought to be passing "this.actor" for onCompleteScope in this case anyway.

Ouch ... I had the impressing that fading was too fast but I did not bother to check this code ... this explains it ;)

> But just do:
> 
>     onComplete: function() { this.actor.hide() },
>     onCompleteScope: this
> 
> it's clearer.)

OK

(In reply to comment #13)
> Well in that case I'd prefer a name like "_showWorkspaceSwitcher" ... we
> already have a "_switchWorkspace" in windowManager.js btw ;)

sure

> > "Fading". one "d". Needs to be fixed everywhere. (including once in 
> 
> ... ok ;) 
> 
> once in what?

oops. I'd originally put the comment with a line from workspaceSwitcherPopup.js, and mentioned that it was misspelled once in windowManager.js too, but then I ended up moving the comment and only deleted half that sentence.

> OK, so I should use different css classes (i.e not referring to
> workspace-switcher at all) for the boxes and spacers?

yeah

> Ouch ... I had the impressing that fading was too fast but I did not bother to
> check this code ... this explains it ;)

You can run with GNOME_SHELL_SLOWDOWN_FACTOR=10, or set Tweener.slowDownFactor in looking-glass, to slow down all the animations to make it easier to see what's happening

Created attachment 153577 [details] [review]
New workspace switcher popup

Fixed up patch.

Created attachment 153661 [details]
arrow without fill gradients

Comment on attachment 153577 [details] [review]
New workspace switcher popup

This is basically good. Two comments:

1. grab Jon's new images before committing

2. The CSS class names are now too generic. Eg:

>+.spacer {

If everyone used names like that, we'd run into naming conflicts between actors in different parts of the shell. You should give them all some unique prefix (like ".ws-switcher-spacer")

Comment on attachment 153577 [details] [review]
New workspace switcher popup

Pushed as abc7b1f, with the new image, and the css fixes.