Created attachment 148154 [details] [review]
[overview] Lightbox the workspaces rather than setting opacity

Review of attachment 148154 [details] [review]:

I agree that the translucent windows looked weired, and lightboxing is way better. There is one small problem with the window captions not being lightboxed, not sure if that falls within the scope of this patch though. Minor nitpicking below.

::: js/ui/workspaces.js
@@ +1506,3 @@
 
+     * @enableLightboxing: Iff true, lightbox all workspaces
+     * setLightBoxAll:

Typo: Iff true ...

@@ +1510,3 @@
+     *
+     * @enableLightboxing: Iff true, lightbox all workspaces
+     * setLightBoxAll:

LightBox is spelled Lightbox everywhere else. Would setLightboxMode be a better name?

Created attachment 155506 [details] [review]
[Overview] Lightbox workspaces on menu popup

Rebased patch

Looks way better.  Is it me or are we not dimming the caption text?  Also can we try making it a little dimmer overall?

(In reply to comment #4)
> Looks way better.  Is it me or are we not dimming the caption text?

No, it's not you (see comment #2) - the same issue occurs with the right-click menu in the app-well.


> Also can we try making it a little dimmer overall?

Yes - is that a change we'd want for all lightboxes (alt-f2, app well right-click menu, more-XXX menus, window preview zoom) or only in this case?

Yeah all of those would be fine.  Thanks.

Created attachment 156277 [details] [review]
[Overview] Lightbox workspaces on menu popup

Slightly darken all lightboxes

Looks pretty good.  Still pretty weird that the captions don't dim.  Not sure we want this until that is fixed.

Also might be good if the effect transition was a bit smoother.  In general we should be using the same fade effect that the window box in the app switcher uses - for more or less everything that appears suddenly.

Created attachment 156362 [details] [review]
[Lightbox] Use a fade effect to smooth the transition

Add show()/hide() methods to Lightbox, which (optionally) fade the
lightbox. Change all lightboxes to fade in smoothly.

Hmm, is it intentional that we dim the active workspace/window here?  E.g. I have one workspace, one Firefox window and I right click on Firefox, it dims the whole thing?

(In reply to comment #10)
> Hmm, is it intentional that we dim the active workspace/window here?  E.g. I
> have one workspace, one Firefox window and I right click on Firefox, it dims
> the whole thing?

Are you referring to the appWell? We do lightboxing there as well, but that's unrelated to this patch ("menu" refers only to the "more-apps"/"more-docs" menus)

Created attachment 156524 [details] [review]
[Overview] Lightbox workspaces on menu popup

Include the dash in the lightboxed area, as its element are disabled anyway.

Some inspiration from bug 613286 - patch depends on attachment 156362 [details] [review].

Review of attachment 156277 [details] [review]:

This looks fine.

Review of attachment 156362 [details] [review]:

::: js/ui/lightbox.js
@@ +95,3 @@
+            this.actor.opacity = 0;
+        if (animate) {
+    show: function(animate) {

I think we should to call Tweener.removeTweens(this.actor) here.  Noticing the  difference in the .2 seconds of animation is admittedly unlikely, but it'd be good to be in the habit of ensuring we only have one transition animation at a time for when it does matter.

@@ +108,3 @@
+            this.actor.opacity = 0;
+        if (animate) {
+    show: function(animate) {

And here.

::: js/ui/runDialog.js
@@ +374,3 @@
+                           time: Lightbox.LIGHTBOX_FADE_TIME,
+                         { opacity: 255,
+        Tweener.addTween(this._group,

I wouldn't expect for other modules to be consuming that constant.  How about adding this._lightBox.getAnimationTime() ?

Review of attachment 156524 [details] [review]:

Looks fine.

Mmmmh - attachment 156277 [details] [review] and attachment 156524 [details] [review] are mutually exclusive; Jon?

Review of attachment 156524 [details] [review]:

Seems to have a bug where the first time I go into all-apps everything is dark including the all-apps box.  Also, if I leave the overview while all-apps is up, the overview is all darkened the next time I come back.  Besides that, I think we also want to keep the arrow and possibly the APPLICATIONS text bright while one is in the all-apps box, as per the mockups.  But yes the rest of dash should also go dark when the all-apps is up.

Created attachment 157522 [details] [review]
[Lightbox] Use a fade effect to smooth the transition

(In reply to comment #14)
> I wouldn't expect for other modules to be consuming that constant.  How about
> adding this._lightBox.getAnimationTime() ?

Mmmmh, not sure - as it's actually the dialog which is animated, I duplicated the constant; there is no real reason for this animation to be of the same length as the "normal" lightbox fade effect.

I'm not really that satisfied with the lightbox.js version in the two latter patches:

 - What darkens seems a bit random - there is stuff that darkens that shouldn't darken (like text Applications and the arrow that reverses), and then maybe the top panel should darken when it doesn't currently?

 - The fade that happens after the menu appears is distracted to my eyes.

I'm thinking for now maybe we should just go with the simple workspaces-only version, which is already a big improvement. 

We can commit that for now, then if Jon likes the effect of the second two patches we could go that way, but it's going to be difficult to implement not darkening Applications and the arrow - I'm not even really sure how to do that now that the dash background is no longer solid black. We can't just cut a rectangle out of lightbox like we used to do with the task list, we'd have to reparent the actors out and raise them above the lightbox or something.

(Jon: it sounds like you applied the second patch without the third patch - the second patch actually depends on the third patch, though the order you apply them doesn't matter. That's the bug you saw - it was throwing an exception when trying to hide the lightbox.)

> +                           time: Lightbox.LIGHTBOX_FADE_TIME,
> +                         { opacity: 255,
> +        Tweener.addTween(this._group,
> 
> I wouldn't expect for other modules to be consuming that constant.  How about
> adding this._lightBox.getAnimationTime() ?

IMO, consuming other modules constants is fine when the constant is conceptually public - adding an accessor doesn't really buy anything but more lines of code.

And I also think that we really do want these two times connected - we want the dialog to fade in together with the background fading out, not two unconnected actions.

Review of attachment 157522 [details] [review]:

Marking as reviewed to get off the unreviewed patches list, since I don't think there's any question about the patch - we just have to decide what part of this we want.

(In reply to comment #19)
> I'm thinking for now maybe we should just go with the simple workspaces-only
> version, which is already a big improvement. 
> 
> We can commit that for now [...]

OK. There's still the problem of the window captions not being darkened, should we ignore that for now? It's probably slightly easier to fix than the text/arrow issue in the alternative patch, but still not trivial.

(In reply to comment #22)
> (In reply to comment #19)
> > I'm thinking for now maybe we should just go with the simple workspaces-only
> > version, which is already a big improvement. 
> > 
> > We can commit that for now [...]
> 
> OK. There's still the problem of the window captions not being darkened, should
> we ignore that for now? It's probably slightly easier to fix than the
> text/arrow issue in the alternative patch, but still not trivial.

Hmm. It's certainly a problem to not darken the captions - what isn't darkened is emphasized. (I didn't notice that trying the patch, but I only had one window so the caption may have been under the app menu.)

Can we easily put a black rectangle over the entire workspaces actor? that's slightly wrong in that it darkens the spacers between the workspaces in grid mode , but I doubt that will be very noticeable.

(In reply to comment #23)
> Can we easily put a black rectangle over the entire workspaces actor? that's
> slightly wrong in that it darkens the spacers between the workspaces in grid
> mode , but I doubt that will be very noticeable.

I did try that at one point - when the overview background was changed from black to dark grey, it was quite noticeable. Maybe I was just too picky, I'll do a patch tomorrow ...

(In reply to comment #24)
> (In reply to comment #23)
> > Can we easily put a black rectangle over the entire workspaces actor? that's
> > slightly wrong in that it darkens the spacers between the workspaces in grid
> > mode , but I doubt that will be very noticeable.
> 
> I did try that at one point - when the overview background was changed from
> black to dark grey, it was quite noticeable. Maybe I was just too picky, I'll
> do a patch tomorrow ...

OK, it was a while ago so I didn't quite remember what the problem was with that approach: the workspaces actor has a fixed size which will be darkened. That's not too much of a problem with a fully populated grid (as it only affects the spacing between workspaces in that case), but it's very noticeable otherwise (e.g. with two workspaces, the entire lower half is darkened)

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Can we easily put a black rectangle over the entire workspaces actor? that's
> > > slightly wrong in that it darkens the spacers between the workspaces in grid
> > > mode , but I doubt that will be very noticeable.
> > 
> > I did try that at one point - when the overview background was changed from
> > black to dark grey, it was quite noticeable. Maybe I was just too picky, I'll
> > do a patch tomorrow ...
> 
> OK, it was a while ago so I didn't quite remember what the problem was with
> that approach: the workspaces actor has a fixed size which will be darkened.
> That's not too much of a problem with a fully populated grid (as it only
> affects the spacing between workspaces in that case), but it's very noticeable
> otherwise (e.g. with two workspaces, the entire lower half is darkened)

Is it pretty easy to figure out what the bounds of the occupied area are?

(In reply to comment #26)
> Is it pretty easy to figure out what the bounds of the occupied area are?

Not sure. If setLightboxMode() is made virtual, the views can implement their specific version; of course the grid view has a pretty clear idea of the bounds of that area. The problem here is that the lightbox is always rectangular, but the covered area is not necessarily so (3, 5, 7, 8, ... workspaces)

Probably the easiest approach would be one lightbox per workspace, but managed by the view - not really nice, but should at least work ...

Created attachment 162871 [details] [review]
[Overview] Lightbox workspaces on menu popup

Rebased on master.

Finally a decision:

Attachment 156524 [details] pushed as 730681a - [Overview] Lightbox workspaces on menu popup

it is ...

Created attachment 162891 [details] [review]
[Lightbox] Use a fade effect to smooth the transition

Updated patch, some changes to the previous version:
  - make fade duration parameter instead of a constant
  - change lightbox constructor to take a hash for optional
    parameters

There is a (trivial) dependency on bug 620775 (for the Overview.PANE_FADE_TIME constant.

Review of attachment 162891 [details] [review]:

::: js/ui/lightbox.js
@@ +46,3 @@
+            if (params.inhibitEvents)
+                inhibitEvents = params.inhibitEvents;
+            if (params.width)

use Params.parse from misc.params for all this (see, eg, the _Draggable constructor in dnd.js), and then just refer to "params.width", etc, rather than copying everything to local variables.

@@ +117,3 @@
 
+    show: function() {
+        Tweener.removeTweens(this.actor);

is this necessary? you don't need to removeTweens if you're overwriting a tween with another tween of the same property

(likewise in hide(), and in runDialog.js)

@@ +213,3 @@
     }
 };
+Signals.addSignalMethods(Lightbox.prototype);

you never emit any signals though...

::: js/ui/runDialog.js
@@ +222,3 @@
+                                               { inhibitEvents: true });
+        this._lightbox.connect('hidden', Lang.bind(this, function() {
+            this._group.hide();

currently a no-op since the lightbox doesn't emit signals...

Created attachment 163201 [details] [review]
[Lightbox] Use a fade effect to smooth the transition

(In reply to comment #31)
> +    show: function() {
> +        Tweener.removeTweens(this.actor);
> 
> is this necessary? you don't need to removeTweens if you're overwriting a tween
> with another tween of the same property
> 
> (likewise in hide(), and in runDialog.js)

Colin suggested adding those calls in comment 14 - remove anyway?

Review of attachment 163201 [details] [review]:

just poked Colin on IRC and he agrees he was confused. so leave out the removeTweens calls.

::: js/ui/lightbox.js
@@ +8,3 @@
+const Tweener = imports.ui.tweener;
+
+const Params = imports.misc.params;

minor nit: js includes should be in a single group (no extra blank line), sorted by namespace (Params before Tweener.)

Created attachment 163204 [details] [review]
[Lightbox] Use a fade effect to smooth the transition

Removed calls to Tweener.removeTweens according to IRC discussion.

Attachment 163204 [details] pushed as 528930d - [Lightbox] Use a fade effect to smooth the transition