Created attachment 207240 [details]
filter layer from the mockups

The filter.

The non linear filter will indeed adds extra depth, please implement it.

I'm pretty sure that patch can be done entirely in CSS ... *hint,hint*

(Off-topic: Allan, you bloody scared me with that subject - I really thought this was about "OMG PLZ BRING BACK GRID VIEW KTHXBYE" ;-)

(In reply to comment #3)
> I'm pretty sure that patch can be done entirely in CSS ... *hint,hint*

I think you can for the lightbox but not for overview anymore since http://git.gnome.org/browse/gnome-shell/commit/?id=155997b5fa9fba95ac5b6f9908af03e63cf2669d .

I'll try to find a way to do it.

Design question: for the overview we should do a radial gradient centered on each monitor right? But the dialogs only the primary monitor gets the gradient? The non-primary ones get the current uniform shade?

FYI: the alpha on the gradient goes from 165 at its darkest to 98 at its most transparent.

Created attachment 207446 [details] [review]
Test implemtnation using GLSL

I've made an implementation of this using a GLSL shader. 
While it seems to work, it doesn't look like a good idea as drag01 told me on IRC that using an effect on a background actor implies the use of a new FBO.

Also, doing this with GLSL excludes graphic card with no support for them.

Do you guys have a precise idea of how it should be done ?

I though of adding a 'shader' property to Clutter's background actor, similar to the dim_factor one, allowing the application (gnome-shell in this case), to specifiy a shader to use in the paint method.
Also, if no-GLSL support is needed, the same effect could be achieved with stencil buffer or using a texture, but I guess this would have to be hardcoded in Clutter source, like the current dim effect.
And with the patch applied, the transition is crude: effect off-effect on, so it would need a property too, modifiable from the app (like dim_factor) and transfered as a uniform (I think) to the shader. Is this doable entirely from gnome-shell ?

(In reply to comment #6)
> Created an attachment (id=207446) [details] [review]
> Test implemtnation using GLSL

> And with the patch applied, the transition is crude: effect off-effect on, so
> it would need a property too, modifiable from the app (like dim_factor) and
> transfered as a uniform (I think) to the shader. Is this doable entirely from
> gnome-shell ?

Yes you can do that see windowManager.js where we use it for dimming windows.

But as I said on IRC using a ShaderEffect implies drawing to an offscreen buffer with the additional performance penalty it brings.

So if we really want to go this road we need at least some numbers to quantity the cost and decide whether it is worth taking it.

(In reply to comment #6)
> Created an attachment (id=207446) [details] [review]
> Test implemtnation using GLSL
> 
> I've made an implementation of this using a GLSL shader. 
> While it seems to work, it doesn't look like a good idea as drag01 told me on
> IRC that using an effect on a background actor implies the use of a new FBO.
> 
> Also, doing this with GLSL excludes graphic card with no support for them.
> 
> Do you guys have a precise idea of how it should be done ?
> 
> I though of adding a 'shader' property to Clutter's background actor, similar
> to the dim_factor one, allowing the application (gnome-shell in this case), to
> specifiy a shader to use in the paint method.
> Also, if no-GLSL support is needed, the same effect could be achieved with
> stencil buffer or using a texture, but I guess this would have to be hardcoded
> in Clutter source, like the current dim effect.
> And with the patch applied, the transition is crude: effect off-effect on, so
> it would need a property too, modifiable from the app (like dim_factor) and
> transfered as a uniform (I think) to the shader. Is this doable entirely from
> gnome-shell ?

What I'd imagine is that if a shader is set, the dim factor is made available as a uniform to the shader. I don't think we need a non-shader fallback to multitexturing - straight dimming should be OK.

Review of attachment 207446 [details] [review]:

Marking needs-work, since the FBO isn't OK for a real implementation

Created attachment 207704 [details] [review]
mutter patch

mutter patch: add user_program property to meta-background-actor

Created attachment 207705 [details] [review]
gnome-shell part of the patch

Updated gnome-shell patch.

We're using the GLSL user_program property to specify a shader to use when drawing the background actor. This avoids using a FBO.
The dim_factor property is bound to a uniform to provide smooth transition.

Created attachment 207706 [details] [review]
gnome-shell part of the patch

(picked up the wrong version of shader in the previous patch)

Review of attachment 207704 [details] [review]:

We're keeping DIM_FACTOR around for cards without GLSL support, right?

::: src/compositor/meta-background-actor.c
@@ +309,3 @@
+  if (priv->user_program != COGL_INVALID_HANDLE)
+  {
+    cogl_material_set_user_program(priv->material,

Missing space before paren.

@@ +317,3 @@
+  else
+  {
+    color_component = (int)(0.5 + opacity * priv->dim_factor);

Move the declaration of color_component to be inside this block if you only use it here.

@@ +390,3 @@
 static void
+meta_background_actor_set_user_program (MetaBackgroundActor *self,
+                                      CoglHandle             user_program)

Funky whitespace.

@@ +491,3 @@
   obj_props[PROP_DIM_FACTOR] = pspec;
   g_object_class_install_property (object_class, PROP_DIM_FACTOR, pspec);
+  /**

Put a space above this line

Review of attachment 207706 [details] [review]:

::: data/shaders/dim-background.glsl
@@ +11,3 @@
+  // the alpha on the gradient goes from 165 at its darkest to 98 at its most transparent.
+  float y = 165.0 / 255.0;
+  float x = 98.0 / 255.0;

IMO the ellipse dimensions and gradient stops should be uniforms.

::: js/ui/overview.js
@@ +121,3 @@
+
+
+

Holy whitespace, Batman!

@@ +647,2 @@
         Tweener.addTween(this._background,
+                         { dim_factor: 0.5,

This is changed... because?

::: src/shell-util.c
@@ +848,3 @@
+
+void
+shell_util_set_program_property(GObject *object, 

Space before declaration. Terrible name ("assign_cogl_shader_program" maybe). Needs a docstring explaining why this is necessary (can't use unboxed types from gjs).

@@ +849,3 @@
+void
+shell_util_set_program_property(GObject *object, 
+                                const char *property_name,

The stars should line up.

@@ +855,3 @@
+  g_value_init (&gvalue, G_TYPE_POINTER);
+  
+  if (clutter_feature_available (CLUTTER_FEATURE_SHADERS_GLSL))

If we don't have CLUTTER_FEATURE_SHADERS_GLSL, we should just be able to bail out without setting anything, right?

@@ +856,3 @@
+  
+  if (clutter_feature_available (CLUTTER_FEATURE_SHADERS_GLSL))
+  {

This is not our whitespace style. Look at some examples of code around you.

@@ +865,3 @@
+
+      g_warning (G_STRLOC ": Unable to compile shader: %s", log_buf);
+      g_free (log_buf);

You want to free the program and the shader and bail out rather than assign a broken program here, right?

@@ +876,3 @@
+
+  g_value_set_pointer (&gvalue, program);
+  g_object_set_property(object, property_name, &gvalue);  

You can lose the GValue shenanigans if you just do:

    g_object_set (object, property_name, program, NULL);

@@ +878,3 @@
+  g_object_set_property(object, property_name, &gvalue);  
+
+  //cogl_handle_unref(program);

Remove this.

It would be great if someone could finish this... ;)

Created attachment 222897 [details] [review]
Add the ability to add shader hooks to MetaBackgroundActor

Using ClutterEffect is not pratical on MetaBackgroundActor, as the FBO
redirection has a noticeable performance impact. Instead, allow adding
GLSL code directly to the pipeline used to draw the background texture.
At the same time, port MetaBackgroundActor to modern Cogl API.

Created attachment 222898 [details] [review]
Implement non-linear overview shade for background

Adding a radial gradent to the dimming effect gives more depth to
the background.
Shading is computed in a GLSL fragment shader, and uses distance to
center of the screen to interpolate the darkening value to use.

Based on a patch by Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

(In reply to comment #16)
> Created an attachment (id=222897) [details] [review]
> Add the ability to add shader hooks to MetaBackgroundActor
> 
> Using ClutterEffect is not pratical on MetaBackgroundActor, as the FBO
> redirection has a noticeable performance impact.

ClutterEffect != ClutterOffscreenEffect

You can add an effect that runs GLSL without redirecting to an FBO, right?

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=222897) [details] [review] [details] [review]
> > Add the ability to add shader hooks to MetaBackgroundActor
> > 
> > Using ClutterEffect is not pratical on MetaBackgroundActor, as the FBO
> > redirection has a noticeable performance impact.
> 
> ClutterEffect != ClutterOffscreenEffect
> 
> You can add an effect that runs GLSL without redirecting to an FBO, right?

No

What's the status of this bug? Does Giovanni's patch need reviewing?

(In reply to comment #20)
> What's the status of this bug? Does Giovanni's patch need reviewing?

Yes will try to find time to do it tomorrow if no one beats me to it.

(In reply to comment #21)
> (In reply to comment #20)
> > What's the status of this bug? Does Giovanni's patch need reviewing?
> 
> Yes will try to find time to do it tomorrow if no one beats me to it.

Sorry for being late have been busy with other stuff.

So I have tested this patch and does not appear to do anything...

Everything looks the same as before.

That's because the shader effect is very subtle. Try with a solid background, and try changing the opacity parameters (x and y in the GLSL code) to 0-255. You'll then notice an ellipse in the middle.
It becomes more evident if the overview is empty, and it's quite noticeable in the animation to/from the overview, as the dim effect is animated radially.

(In reply to comment #23)
> That's because the shader effect is very subtle. Try with a solid background,
> and try changing the opacity parameters (x and y in the GLSL code) to 0-255.
> You'll then notice an ellipse in the middle.
> It becomes more evident if the overview is empty, and it's quite noticeable in
> the animation to/from the overview, as the dim effect is animated radially.

Oh OK now I am seeing it.

Review of attachment 222897 [details] [review]:

Looks good.

Review of attachment 222898 [details] [review]:

Looks good.

::: js/ui/overview.js
@@ +635,2 @@
         Tweener.addTween(this._background,
+                         { dim_factor: 0.8,

While we are changing this one ... we should use a constant rather then a hardcoded number.

Attachment 222897 [details] pushed as 859ea14 - Add the ability to add shader hooks to MetaBackgroundActor

Attachment 222898 [details] pushed as 025c63c - Implement non-linear overview shade for background

Yaaaaaay! Really looking forward to seeing this.

MetaShaderHook is an awful hack. You should have pinged the cogl people on this inconsistency.

I'd like to play around with the gradient here. (Specifically, I'd like to try making it a bit darker around the edges.) Any pointers on which values to change?

(In reply to comment #31)
> I'd like to play around with the gradient here. (Specifically, I'd like to try
> making it a bit darker around the edges.) Any pointers on which values to
> change?

Play with x / y in the shader (overview.js)

The shade with the radial gradient was supposed to be for both the overview *and* modal dialogs. Looking at it, it only seems to be happening for the overview...

Applying it to modal dialogs turned out be more difficult than I expected, and in the end I took the short route of a ClutterOffscreenEffect, but here it is.

Created attachment 231604 [details] [review]
Shell: introduce a new ShellShaderEffect class

This is like ClutterShaderEffect (a ClutterEffect for applying arbitrary
GLSL code), except that it exposes and uses the CoglSnippet API instead
of the legacy CoglShader one, so that we don't override Cogl internal
GLSL generation. In particular, it gets us texture lookup for free.
The design is based on the similar API that's exposed by MetaBackgroundActor.

Created attachment 231605 [details] [review]
Add a radial background shade for modal dialogs

Use the new ShellShaderEffect to build a RadialEffect that can be enabled
on Lightboxes to achieve a radial effect similar to the overview one.
Then enable it for modal dialogs.

The shader parameters may need tweaking, in their current form the effect is barely noticeable.

(In reply to comment #37)
> The shader parameters may need tweaking, in their current form the effect is
> barely noticeable.

At this point the question is ... is it really worth all the complexity and performance penalty for an effect that most users wont notice anyway?

Review of attachment 231604 [details] [review]:

yuck

(In reply to comment #39)
> Review of attachment 231604 [details] [review]:
> 
> yuck

Asd. I kind of expected this reaction.
If that will make you feel better, with ShellShaderEffect I can turn ShellInvertLightnessEffect into 10 lines of JS + GLSL.

(In reply to comment #39)
> Review of attachment 231604 [details] [review]:
> 
> yuck

So, is yuck your final answer?

Review of attachment 231604 [details] [review]:

::: src/shell-shader-effect.c
@@ +13,3 @@
+ * to implement add_snippets()
+ *
+ * #ShellShaderEffect is available since Shell 1.4

s/Cogl/Shell/g mistake

@@ +143,3 @@
+  /* Note that, differently from ClutterBlurEffect, we are calling
+     this inside constructed, not init, so klass points to the most-derived
+     GTypeClass, not ShellShaderEffectClass.

This is a common pattern, so I'm not sure you need to explain anything here.

@@ +148,3 @@
+  self = SHELL_SHADER_EFFECT (object);
+
+  if (G_UNLIKELY (klass->base_pipeline == NULL))

Why is this very unlikely, especially considering the shell takes this path? Branch prediction is fine here.

Also, do you expect anything to use base_pipeline?

@@ +153,3 @@
+        clutter_backend_get_cogl_context (clutter_get_default_backend ());
+
+      klass->base_pipeline = cogl_pipeline_new (ctx);

Cogl's pipeline system is based around so that it can share programs between pipelines. See _st_create_texture_material for how to take advantage of this using a copy.

@@ +199,3 @@
+void
+shell_shader_effect_add_glsl_snippet (ShellShaderEffect *effect,
+                                      MetaSnippetHook    hook,

We should file an upstream bug about this not being exposed in the experimental API.

(In reply to comment #42)
> Review of attachment 231604 [details] [review]:
> 
> ::: src/shell-shader-effect.c
> @@ +13,3 @@
> + * to implement add_snippets()
> + *
> + * #ShellShaderEffect is available since Shell 1.4
> 
> s/Cogl/Shell/g mistake
> 
> @@ +143,3 @@
> +  /* Note that, differently from ClutterBlurEffect, we are calling
> +     this inside constructed, not init, so klass points to the most-derived
> +     GTypeClass, not ShellShaderEffectClass.
> 
> This is a common pattern, so I'm not sure you need to explain anything here.

It is not: all other Clutter*Effect classes initialize their pipelines inside init, and klass there always points to the same GTypeClass - which means pipelines are shared by all instances and all subclasses (unless the subclass overrides).
This is not true for ShellShaderEffect classes, because you can't call a JS virtual before construct properties are set.

> @@ +148,3 @@
> +  self = SHELL_SHADER_EFFECT (object);
> +
> +  if (G_UNLIKELY (klass->base_pipeline == NULL))
> 
> Why is this very unlikely, especially considering the shell takes this path?
> Branch prediction is fine here.

Because klass->base_pipeline is NULL only once in the lifetime of the program. All instances except the first (and I expect many of them) take the != NULL path, so that's the flow to optimize for.

> Also, do you expect anything to use base_pipeline?
>
> @@ +153,3 @@
> +        clutter_backend_get_cogl_context (clutter_get_default_backend ());
> +
> +      klass->base_pipeline = cogl_pipeline_new (ctx);
> 
> Cogl's pipeline system is based around so that it can share programs between
> pipelines. See _st_create_texture_material for how to take advantage of this
> using a copy.

Sharing programs makes sense if the have the same code, so using _st_create_texture_material would buy nothing here, because we're adding snippets to it, forcing Cogl to regen the GLSL and mesa to recompile it.
But we are taking advantage of cogl_pipeline_copy: one master pipeline is created for the first instance, filled with snippets, samplers, etc. The master pipeline is stored in the class, and every other instance just references that state.
 
> @@ +199,3 @@
> +void
> +shell_shader_effect_add_glsl_snippet (ShellShaderEffect *effect,
> +                                      MetaSnippetHook    hook,
> 
> We should file an upstream bug about this not being exposed in the experimental
> API.

None of the experimental API is exposed in the GIR file. I don't know if they consider that a bug, but they don't seem to very keen on supporting introspection...

Created attachment 234920 [details] [review]
Remove some unneeded imports

They cause circular module dependencies, which end up with wrong prototype
inheritance.

Created attachment 234921 [details] [review]
Gdm: move LogoMenuButton from LoginDialog to a separate module

The dependency chain is Main -> EndSessionDialog -> ModalDialog ->
Lightbox -> Overview -> ViewSelector -> Wanda -> Panel -> LoginDialog.
Thus, when LoginDialog is imported, ModalDialog is only partially
loaded, and ModalDialog.ModalDialog cannot be inherited from.

Review of attachment 234920 [details] [review]:

How could circular module dependencies end up with wrong prototype inheritance?

Review of attachment 234921 [details] [review]:

Lightbox doesn't import Overview?

(In reply to comment #47)
> Review of attachment 234921 [details] [review]:
> 
> Lightbox doesn't import Overview?

It would, if you reviewed the previous patches. Which is why I posted this here, rather than filing a new bug.

(In reply to comment #46)
> Review of attachment 234920 [details] [review]:
> 
> How could circular module dependencies end up with wrong prototype inheritance?

Look at the other commit message. By the time the code in loginDialog.js is evaluated, modalDialog.js is still importing its own dependencies, and hasn't created the ModalDialog class yet.

(In reply to comment #45)
> Created an attachment (id=234921) [details] [review]
> Gdm: move LogoMenuButton from LoginDialog to a separate module
> 
> The dependency chain is Main -> EndSessionDialog -> ModalDialog ->
> Lightbox -> Overview -> ViewSelector -> Wanda -> Panel -> LoginDialog.
> Thus, when LoginDialog is imported, ModalDialog is only partially
> loaded, and ModalDialog.ModalDialog cannot be inherited from.

I'd prefer if we put the GLSL declarations in Lightbox, and import them from Overview. Or put them in a new module.

So, with the new background changes, what are we doing with this?

Modal dialogs have little to do with the background (ok, they shared the shader code). I guess I'll have to rebase, and then you can review again.

Created attachment 238005 [details] [review]
Shell: introduce a new ShellShaderEffect class

This is like ClutterShaderEffect (a ClutterEffect for applying arbitrary
GLSL code), except that it exposes and uses the CoglSnippet API instead
of the legacy CoglShader one, so that we don't override Cogl internal
GLSL generation. In particular, it gets us texture lookup for free.
The design is based on the similar API that's exposed by MetaBackgroundActor.

Created attachment 238006 [details] [review]
Add a radial background shade for modal dialogs

Use the new ShellShaderEffect to build a RadialEffect that can be enabled
on Lightboxes to achieve a radial effect similar to the overview one.
Then enable it for modal dialogs.

Comment on attachment 234921 [details] [review]
Gdm: move LogoMenuButton from LoginDialog to a separate module

Not needed anymore

Review of attachment 238005 [details] [review]:

"The design is based on the similar API that's exposed by MetaBackgroundActor."

Not anymore isn't it.

::: src/shell-shader-effect.c
@@ +195,3 @@
+ *
+ * This is only valid inside the a call to the build_pipeline() virtual
+ * function.

Perhaps you should add some validation for this?

Review of attachment 238006 [details] [review]:

OK.

This was fixed.

(In reply to comment #57)
> This was fixed.

Not for modal dialogs. But ok for me if you no longer care.

(In reply to comment #58)
> Not for modal dialogs. But ok for me if you no longer care.

Oh, my bad. Forgot about that.

Created attachment 257185 [details] [review]
Add a radial background shade for modal dialogs

Use a new ShellGLSLQuad actor class to build a RadialEffect that can be
enabled on Lightboxes to achieve a radial effect similar to the overview
one. Then enable it for modal dialogs.

Now without FBOs!

I don't really notice any difference with that patch applied ....

(In reply to comment #61)
> I don't really notice any difference with that patch applied ....

That's because the parameters need tweaking (with the current values, most of the vignette is behind the dialog and goes unnoticed).

Review of attachment 257185 [details] [review]:

Code looks good, but the result needs to be tweaked.

Designers are never going to tweak this if they don't see it,
let's push it and fix it later.

Attachment 257185 [details] pushed as 7e9ecf4 - Add a radial background shade for modal dialogs

Yay! Thanks for this! I've tried it and it seems a bit faint.

Assuming we are not doing so already, maybe we could try using the same vignette as in the overview?