Created attachment 193737 [details] [review]
overview: make this._group fill the screen instead of only the primary

(In reply to comment #0)
> There shouldn't be any other side-effects since the overview's remaining
> actors don't rely on this._group's size.

But you change the position, which will cause side effects.

There's a few other solutions:

 * A new actor that stretches from (0, 0) to (screen_width, screen_height) with the fade, and putting this._group inside of that.

 * A separate actor in the overlay group that we'll fade, and we remove the background-color from #overview. See lightbox.js

(In reply to comment #2)
> (In reply to comment #0)
> > There shouldn't be any other side-effects since the overview's remaining
> > actors don't rely on this._group's size.
> 
> But you change the position, which will cause side effects.

Indeed. *facepalm*

> There's a few other solutions:
> 
>  * A new actor that stretches from (0, 0) to (screen_width, screen_height) with
> the fade, and putting this._group inside of that.
> 
>  * A separate actor in the overlay group that we'll fade, and we remove the
> background-color from #overview. See lightbox.js

Nod. Will submit something tomorrow when I'm with my 2nd display again.

Created attachment 194076 [details] [review]
overview: make the background of non-primary monitors darkened

This makes the overall overview appearance consistent across all monitors.

Review of attachment 194076 [details] [review]:

This looks like it works, but there's an alternate way of approaching this that adds a huge amount of efficiency when going into the overview that I'd really rather do instead.

Right now, what we do is:
 
 Read background pixel from texture 4 bytes
 Write to frame buffer              4 bytes
 Read pixel from frame buffer       4 bytes
 Blend with black
 Write result to frame buffer        4 bytes

But note that what we are dimming to black is basically just the background actor (*), so what we should do is:

 Read background pixel from texture 4 bytes
 Blend with black
 Write result to frame buffer       4 bytes

Saving us (pixels in screen) * 8 bytes of memory bandwidth for every frame we draw going into the overview.

We can add a dim-fraction property to MetaBackgroundActor and just implement implement it by changing the material for MetaBackgroundActor to do the appropriate cogl blending.

(*) Not completely true - when desktop icons are enabled, during the animation we mix the desktop icons with the background and fade that, but there's no UI to turn them on, and so in that case, I think we can just accept the change from dim(interp(icons, plain background)) to interp(icons, dim(plain_background)) - it will mean that you get a more abrupt dim in the with-icons case than without, but performance in the default case is more important to me than that.

Created attachment 194870 [details] [review]
MetaBackgroundActor: add a dim-factor property

Created attachment 194871 [details] [review]
Overview: use the background_actor dim-factor property to dim

Created attachment 194874 [details] [review]
WorkspaceThumbnail: set the background dim-factor temporarily while painting

Created attachment 194877 [details] [review]
MetaBackgroundActor: add a dim-factor property

--
I removed the queue_relayout (which should have been a queue_redraw to begin
with) from the property setter and it seems to work still. I'm not really sure
why though :-)

Hmmm, I'm basically not willing to do the hack where we have a dim-factor item that doesn't trigger a redraw ... that's a bad hack that spills over between GnomeShell and Mutter. I'm OK with bad hacks, but only when encapsulated :-)

(And queuing redraws out of ::paint is just bad news - I think the only reason you didn't go into an infinite loop was the short-circuit in set_dim_factor() that returns immediately when the value is the same as the old one.)

I'll attach a rework of MetaBackgroundActor that allows multiple copies while sharing the same CoglTexture. You'll need to, beyond what I'm attaching here:

 - Split meta-background-actor.h into a -private.h and a public .h that goes into the src/meta/ subdirectory.
 - Rebase your ::dim-factor property on top of this (should be easy)

Also, review of my patch and additional testing would definitely be appreciated!

Created attachment 194979 [details] [review]
MetaBackgroundActor: allow creating multiple instances

Instead of requiring a singleton MetaBackgroundActor for the screen,
allow creating multiple copies that internally share a single
CoglTexture behind the scenes. This will be useful for allowing
multiple views of the screen background with different rendering
options.

Created attachment 195024 [details] [review]
MetaBackgroundActor: add a dim-factor property

--
- rebased on top of Owen's patch

Created attachment 195025 [details] [review]
MetaBackgroundActor: make this a public class

Created attachment 195026 [details] [review]
Use Meta.BackgroundActor instances instead of cloning global.background_actor

Instances of this class share a single CoglTexture behind the scenes which
allows us to show the background with different rendering options without
duplicating the texture data.

Created attachment 195027 [details] [review]
Overview: dim the background with the dim-factor property

This saves us (pixels in screen) * 8 bytes of memory bandwidth for every frame
we draw going into the overview.

It also allows us to dim the background on non-primary monitors.

Review of attachment 194979 [details] [review]:

This is really nice Owen, thanks.

I tested it alone and with each one of my patches on top. No problems.

Comment on attachment 194979 [details] [review]
MetaBackgroundActor: allow creating multiple instances

Attachment 194979 [details] pushed as c630046 - MetaBackgroundActor: allow creating multiple instances

Review of attachment 195024 [details] [review]:

Good to commit except for the following:

::: src/compositor/meta-background-actor.c
@@ +299,3 @@
   meta_screen_get_size (self->background->screen, &width, &height);
 
+  color_component = 255 * self->dim_factor;

1) you need to take opacity into account here too
2) although this sort of conversion is pretty commonly found, I dislike having a dim_factor of 0.99999 give you a color_component of 254
 
 color_component = (int)(0.5 + opacity * self->dim_factor);

@@ +363,3 @@
+  g_object_notify (G_OBJECT (self), "dim-factor");
+
+  clutter_actor_queue_redraw (CLUTTER_ACTOR (self));

Doesn't matter here , but as a matter of consistency, make the g_object_notify() the last line of the function, since notifcations should happen after everything else is done.

Review of attachment 195024 [details] [review]:

Oh, can you make the commit message just a bit more descriptive as well - mention that the property darkens the background or something.

Review of attachment 195025 [details] [review]:

Looks fine.

(It would also have been fine to simply not make the structures public and keep them private to the .c file.)

Review of attachment 195026 [details] [review]:

Looks good

Review of attachment 195027 [details] [review]:

Change "This" at the beginning of the commit message to:

 "Doing this rather than overdrawing a black rectangle "

Otherwise good to commit

Created attachment 195078 [details] [review]
MetaBackgroundActor: add a dim-factor property

This property darkens the background.

--
- Thanks for the review, comments addressed;

- Did a little optimization by keeping the property GParamSpec and use
  notify_by_pspec.

Review of attachment 195078 [details] [review]:

Don't think it's a worthwhile complication^H^H^H^H^optimization except for the very most commonly notiifed properties, but sure.

(In this case, the notification isn't actually needed because it's done automatically at property set time, I didn't comment it before because it would be needed if the setter was ever made into a public function or otherwise called from somewhere else in the code.)

Comment on attachment 195078 [details] [review]
MetaBackgroundActor: add a dim-factor property

Attachment 195078 [details] pushed as 53e70b3 - MetaBackgroundActor: add a dim-factor property

Comment on attachment 195025 [details] [review]
MetaBackgroundActor: make this a public class

Attachment 195025 [details] pushed as b28c653 - MetaBackgroundActor: make it a public class

[ hand edit because I changed the patch summary slightly and git-bz doesn't cope with that ]

Attachment 195026 [details] pushed as 82ce8fe - Use Meta.BackgroundActor instances instead of cloning global.background_actor
Attachment 195027 [details] pushed as 155997b - Overview: dim the background with the dim-factor property