After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 609187 - Fancier workspace switcher popup
Fancier workspace switcher popup
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-02-06 20:48 UTC by drago01
Modified: 2010-02-12 22:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
New workspace switcher popup (6.40 KB, patch)
2010-02-07 12:26 UTC, drago01
none Details | Review
New workspace switcher popup (34.32 KB, patch)
2010-02-07 16:54 UTC, drago01
none Details | Review
New workspace switcher popup (15.20 KB, patch)
2010-02-07 17:28 UTC, drago01
none Details | Review
New workspace switcher popup (15.56 KB, patch)
2010-02-07 19:46 UTC, drago01
none Details | Review
New workspace switcher popup (15.56 KB, patch)
2010-02-07 20:07 UTC, drago01
none Details | Review
New workspace switcher popup (15.56 KB, patch)
2010-02-07 20:34 UTC, drago01
none Details | Review
New workspace switcher popup (15.85 KB, patch)
2010-02-07 21:46 UTC, drago01
none Details | Review
New workspace switcher popup (15.87 KB, patch)
2010-02-08 20:44 UTC, drago01
needs-work Details | Review
New workspace switcher popup (15.83 KB, patch)
2010-02-11 21:18 UTC, drago01
committed Details | Review
arrow without fill gradients (15.66 KB, image/svg+xml)
2010-02-12 20:20 UTC, William Jon McCann
  Details

Description drago01 2010-02-06 20:48:48 UTC
Currently we still use the old metacity popup for ctrl-alt-arrow, which really does not fit into the rest of the shell's design.

We should replace it with something that looks more like the alt-tab dialog, maybe similar to the one used by compiz's wall plugin.
Comment 1 drago01 2010-02-07 12:26:45 UTC
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 ;) )
Comment 2 drago01 2010-02-07 16:54:36 UTC
Created attachment 153211 [details] [review]
New workspace switcher popup

New version based on the mockups from:

http://www.gnome.org/~mccann/screenshots/clips/20100207160938/
Comment 3 drago01 2010-02-07 17:28:14 UTC
Created attachment 153214 [details] [review]
New workspace switcher popup

*) Use 96x96 icons to be closer to the alt-tab dialog
Comment 4 William Jon McCann 2010-02-07 18:15:08 UTC
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.
Comment 5 drago01 2010-02-07 19:46:26 UTC
Created attachment 153225 [details] [review]
New workspace switcher popup

Fixed up patch.

*) better spacing
*) flipped the image rather than rotating it
*) code cleanups
Comment 6 William Jon McCann 2010-02-07 20:01:14 UTC
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;
 }
Comment 7 drago01 2010-02-07 20:07:27 UTC
Created attachment 153227 [details] [review]
New workspace switcher popup

Added CSS changes from comment 6
Comment 8 drago01 2010-02-07 20:34:46 UTC
Created attachment 153229 [details] [review]
New workspace switcher popup

*) Minor cleanup
Comment 9 drago01 2010-02-07 21:46:44 UTC
Created attachment 153231 [details] [review]
New workspace switcher popup

Make sure that altTab and the workspaceSwicher never show up at once.
Comment 10 drago01 2010-02-08 20:44:26 UTC
Created attachment 153291 [details] [review]
New workspace switcher popup

Rebased to current head.
Comment 11 Dan Winship 2010-02-11 19:12:53 UTC
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.)
Comment 12 Dan Winship 2010-02-11 19:40:21 UTC
(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.)
Comment 13 drago01 2010-02-11 19:54:53 UTC
(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
Comment 14 Dan Winship 2010-02-11 20:01:13 UTC
(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
Comment 15 drago01 2010-02-11 21:18:24 UTC
Created attachment 153577 [details] [review]
New workspace switcher popup

Fixed up patch.
Comment 16 William Jon McCann 2010-02-12 20:20:05 UTC
Created attachment 153661 [details]
arrow without fill gradients
Comment 17 Dan Winship 2010-02-12 21:44:07 UTC
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 18 drago01 2010-02-12 22:54:32 UTC
Comment on attachment 153577 [details] [review]
New workspace switcher popup

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