I did not like the Metacity top light shadow, which is why it's top-left :) I am thinking that since we are extending the theme, perhaps the type of the shadow should be themeable.

I'm in agreement that the shadow parameters should be part of the theme - the right shadow is going to depend in part on what the borders look like.

I just wanted to update this bug with a status update.  Volker has done some good work writing a pretty nice shadow effect.  I helped him today do some modifications to mutter-window.c so we can control the shadow on a per window basis if we want to.  Things still need a bit more tweaking but I will attach a screen shot for some comments.  The parameters we have tentatively added are shadow_radius, shadow_offset_x, shadow_offset_y, shadow_opacity.

Created attachment 143476 [details]
quick progress screen-capture

Created attachment 144621 [details] [review]
(1/4) For the series of shadow enhancement patches

Created attachment 144623 [details] [review]
(2/4) For the series of shadow enhancement patches

Created attachment 144624 [details] [review]
(3/4) For the series of shadow enhancement patches

Created attachment 144625 [details] [review]
(4/4) For the series of shadow enhancement patches

Review of attachment 144623 [details] [review]:

Your properties don't actually do anything since they aren't hooked up. 

Since we want to do this in the theme long-term, I think it's better to keep things simple and have:

 meta_comositor_set_shadow_properties(compositor, radius, opacity, offset_x, offset_y);

That would store the shadow on hte compositor, then have a private:

 mutter_window_update_shadow(window)

that that function can can loop through and call. You don't need accessors.... MutterWindow already includes compositor-private.h.

One advantage of doing it that way, is that you'll avoid dealing with the problem in the current code can't you can't actually have different shadows for different windos, since they all share the same window texture.

Review of attachment 144625 [details] [review]:

::: src/compositor/compositor.c
@@ +1087,3 @@
+   */
+  compositor->shadow = g_new(MutterShadow,1);
+  mutter_shadow_init_defaults(compositor->shadow);

As noted elsewhere, I'm not sure we want the complexity of per-window shadow properties, yes.

::: src/compositor/mutter-window.c
@@ +547,3 @@
+                                    w + priv->shadow->radius * 2 - priv->shadow->offset_x,
+                                    h + priv->shadow->radius * 2 - priv->shadow->offset_y);
+            clutter_actor_set_opacity (priv->shadow->actor, priv->shadow->opacity);

As this gets more complicated, it really needs to be factored out, instead of repeated inline here in the set-property function.

@@ +560,1 @@
     default:

Well, this is slightly better before when this wasn't here at all, but still not very useful...

::: src/compositor/shadow.c
@@ +40,3 @@
+void
+mutter_create_shadow_frame (MutterShadow *shadow,
+                            MutterShadow *shadow_default)

My understanding of this function is that it would need to be documented something like:

 If the shadow parameters in shadow match the shadow do not match the parameters
 in shadow_default, then the parameters in shadow_default are updated and a
 a new actor is stored in shadow_default->actor. Then a new shadow texture actor
 is stored in shadow->actor.

This is too messy. The function should have a simple prototype like:

 ClutterActor *
 mutter_create_shadow_frame (MetaCompositor *compositor,
                             MutterShadow   *shadow);

That can be documented as:

 Creates a texture frame actor based on the shadow parameters in @shadow

And any caching should be behind the scenes.

@@ +51,1 @@
+      shadow_default->actor = clutter_texture_new ();

Looks like the old shadow actor is leaked

@@ +133,3 @@
       data[-i] = tmp;
     }
+

Bunch of whitespace changes here and below

::: src/compositor/shadow.h
@@ +8,3 @@
 #define SHADOW_OPACITY  0.8
 #define SHADOW_OFFSET_X 0
+#define SHADOW_OFFSET_Y 5

This probably needs some sort of comment like "We default to a shadow that looks like it is coming from a lightsource slightly above" (if that's what is on)

@@ +12,3 @@
+typedef struct _MutterShadow
+{
+  ClutterActor    *actor;

I think having the actor in here, and then having it be:

 - A clutter texture for the "default shadow"
 - A TidyTextureFrame in other usage

Is pretty confusing. (If I'm understanding the code right)

@@ +20,3 @@
+
+void mutter_shadow_init_defaults (MutterShadow *shadow);
+void mutter_create_shadow_frame (MutterShadow *shadow, MutterShadow *shadow_default);

Prototypes need to be one parameter per line and lined up.

Review of attachment 144624 [details] [review]:

::: src/compositor/mutter-window.c
@@ +556,2 @@
             clutter_actor_set_size (priv->shadow, w + priv->shadow_radius * 2,
+                                    h + priv->shadow_radius * 2);

I don't see what the size of the shadow texture has to do with the position of the shadow texture.

::: src/compositor/shadow.c
@@ +67,3 @@
                                   margin);
 
+  clutter_actor_set_anchor_point (frame, margin - 1, margin - 1);

I think it's better to avoid changing the anchor point. anchor points have some weird effects, and it's easier to just leave actors positioned from the top left.

@@ +198,3 @@
 	}
     }
+

Whitespace change

::: src/compositor/shadow.h
@@ -10,1 +10,1 @@
 #define SHADOW_OFFSET_Y 0 

The patch changes the anchor point in both X and Y, but only changes these #defines in X?

Review of attachment 144621 [details] [review]:

Various comments, basically once I got down to make_shadow_texture_rgb_data() I got completely lost, however. The code wasnt' doing anything like what I expected.

My understanding of what we want to compute is the convolution of a corner with a Gaussian kernel. The algorithm I'd expect for that is something like:

 - Create an array which is the convolution of a "step" with the Gaussian kernel - this will be of equal length to the window size.
 - Cross-multiply by itself to get a corner
 - Replicate 4 times to get the 4 corners
 - Convert to uchar
 - Expand to an argb texture (this shouldn't be necessary actually - a single channel texture should work fine)

(Some of these steps can obviously be combined)

Many of these things seem to be present in your code except for the convolution with the step. Your code seems to assume that the convolution of a step with a guassian is half a Guassian, but that can't be the case because the convolution of a step with a Guassian is going to be symetric around the step edge - the value of the convolution will be 0.5 at the edge and the part to the left and to the right will look like like each other but inverted.

(So this means that the shadow in your code will be darker under the edge of the window then it should be)

I'm really not quite sure what is going on with the "data_center" in your patch. Replicating a corner piece 4 times will give you an even size texture with the two center rows/columns the same. If you need to get an odd-sized texture, then you can simply fix that up and replicate the center row/column an extra time in the final texture data.

Sorry for being a little sketchy here - I find it hard to explain this stuff without the benefit of drawing it. It may also be that I'm misunderstanding your code or missing some clever trick that you are doing. In this case, comments in the code would help :-)

::: src/compositor/shadow.c
@@ +14,3 @@
+
+#define SHADOW_RADIUS   30
+#define SHADOW_OPACITY	0.8

Once the shadow opacity is one of the parameters of the shadow and we use clutter_actor_set_opacity() to modulate it, I don't think you want the opacity here as well.

@@ +18,3 @@
 #define SHADOW_OFFSET_Y	(SHADOW_RADIUS)
 
+/* guchar: 0..255 */

guchar really isn't relevant here.

@@ +32,3 @@
+ClutterActor *          mutter_create_shadow_frame (MetaCompositor *);
+static GaussianKernel * make_gaussian_kernel (gint);
+static guchar *         make_shadow_texture_rgb_data (gint, gint *, gint *);

Prototypes need the parameter names and should be line broken with one parameter per line and everything lined up.

@@ +60,3 @@
     }
 
+  /* FIXME: better use nbtk-texture-frame instead of the older tidy-texture-frame? */

Not a useful fixme to have in the sources. 

I don't think there is much differnece between cut-and-pasting TidyTextureFrame and cut-and-pasting NbtkTextureframe (MxTextureFrame) in, but if we want to switch, that should be filed as a bug.

(NbtkTextureFrame shares the same weirdness as TidyTextureFrame - the source is a ClutterTexture not a CoglTexture.)

@@ +71,1 @@
+  /* FIXME: is it possible to return clutter_clone_new(frame) instead? */

I don't think so, no. TidyTextureFrame is already a "clone" of the shadow_src. There is no reason to clone it again.

@@ +99,3 @@
+  /* FIXME: Could we do this this better, so that for a given shadow
+     radius n we really get a shadow with a width of n pixels which
+     are all (except one?) different from 0 ? */

But the kernel that you are computing here is doubles? In doubles, the extent of the Gaussian kernel is infinite right? So when would you get pixels that are 0?

@@ +102,3 @@
+  /* Calculate $sigma2 so that 255*exp(-($radius^2/$sigma2)) = 1,
+     that is, for one pixel with a value of 255 at the distance
+     $radius from the center and everything else 0, we get 1.

Hmm, I think what you are trying to get at here is "how far do we have to go out to get a pixel that doesn't matter in the final result".

But this seems backward to me - sigma is the primary parameter determining the spread of the Guassian and what should be passed in. Not the width of the "window" - that should be computed based on Sigma.

You could do that computation like this trying to figure out "what doesn't matter", but I think that's going to give you a very, very big window (if one pixel at radius gives you a contribution that's < 1/255 you can still add up multiple pixels to give a noticeable result). It's probably better to just use a fixed fraction of sigma. (a window of 5 sigma - 2.5 sigma in either direction - sticks in my mind as something that is commonly used? But you probably could get away with less than that.)

If "ringing" from windowing the Guassian is a problem, then it's probably better to use a better window function rather than using a gigantic window.

@@ +108,3 @@
+  /*sigma2 = radius * radius / log (255.0);*/
+  /* we could also consider the opacity too: */
+  sigma2 = radius * radius / log (SHADOW_OPACITY * 255.0);

This will need to be cleaned up before the patch goes in.

@@ +111,3 @@
+
+  /* center the pointer */
+  data = data + center;

offsetting data then offsetting it back is confusing. Just write data[center + i], data[center - i]. Performance will be not significantly different.

@@ +128,3 @@
+      tmp      = data[i] / sum;
+      data[i]  = tmp;
+      data[-i] = tmp;

If you were going to do this, then why assign both data[i] and data[i-1] to begin with? (I'd probably just walk over all of data to keep things simple.. computing the 1-D kernel is not going to be the limiting factor)

@@ +141,3 @@
+  printf ("  size = %d\n", size);
+  printf ("radius = %d\n", center);
+  printf ("sigma2 = %f\n\n", sigma2);

Will need to be cleaned up.

@@ +152,3 @@
+  gdouble        *kdata         = kernel->data;
+  /* The texture will be two pixels wider/higher than the kernel
+     becase we need a three pixel wide, evenly equally collored center */

spelling 'colored'

@@ +163,3 @@
+
+  guchar          color_rgba[4] = {COLOR_RED, COLOR_GREEN, COLOR_BLUE, 0};
+  guint32        *color         = (guint32 *) color_rgba;

This isn't endian safe

I'd like to mention that i haven't been working on this for quite some time now and the changes i made in shadow.c on my gitorious are (obviously) cluttered with 'personal' debugging stuff and comments and far from beeing ready to be used for a patch, that's why i didn't yet post a patch myself.

I argree with the comments Owen has made. I've already fixed one thing he pointed out:

* "(So this means that the shadow in your code will be darker under the edge of
the window then it should be)"
=> There is a more recent version on http://gitorious.org/gnome-shell-sandbox/mutter (there's only one branch, called shadow-fix) where the 'inside' part of the blur is calculated too.

However, this introduces a problem with windows that are smaller than a certain size, where the margins of the TidyTextureFrame start to overlap. I haven't yet found a solution for this myself. This would need to be fixed first of all, i think.

A few extra thoughts as I was driving home after writing the above.

1)

> You could do that computation like this trying to figure out "what doesn't
> matter", but I think that's going to give you a very, very big window (if one
> pixel at radius gives you a contribution that's < 1/255 you can still add up
> multiple pixels to give a noticeable result). It's probably better to just use
> a fixed fraction of sigma. (a window of 5 sigma - 2.5 sigma in either direction
> - sticks in my mind as something that is commonly used? But you probably could
> get away with less than that.)

Because the figure you are convolving is convex, the "add up multiple pixels" thing actually can't occur so your criterion is correct.

And what is sqrt(-log(1/255.)) ? 2.35. So basically identical to the 2.5 I suggested as an arbitary number...

I still think it makes more sense to paramerize with sigma rather than the total size.

2) 'the convolution of a "step" with the Gaussian kernel' isn't as complex as it sounds - it's just summing up the kernel. So, if the kernel was

  0.1, 0.2, 0.4, 0.2, 0.1

Then the convolution would be:

  0.1, 0.3, 0.7, 0.9, 1.0

Since it's so simple I'm worried that I missed it somewhere in the patch But I still don't see it in a second look :-)

3) Like any review, I concentrated on the stuff that *didn't* make sense to me. But I don't want to come across like the patches didn't make sense at all. Thanks a lot for tackling this and coming up with something!

Something to bear in mind, which no other OS seems to get right:

When two shadows overlap, the result should be no darker than a single shadow.

Steps to reproduce:
1. Put one object (e.g. a hand) above a surface so that it casts a shadow.
2. Add another object just above it.

While the extent of the shadow on the surface may change, it doesn't get any darker.

There's also a second shadow cast on the lower object.

If the light source is sufficiently nearby (e.g. a lamp versus the Sun) then an object nearer to the surface will cast a less diffuse, darker shadow.

(In reply to comment #15)
> Something to bear in mind, which no other OS seems to get right:
> 
> When two shadows overlap, the result should be no darker than a single shadow.
> 
> Steps to reproduce:
> 1. Put one object (e.g. a hand) above a surface so that it casts a shadow.
> 2. Add another object just above it.
> 
> While the extent of the shadow on the surface may change, it doesn't get any
> darker.
> 
> There's also a second shadow cast on the lower object.
> 
> If the light source is sufficiently nearby (e.g. a lamp versus the Sun) then an
> object nearer to the surface will cast a less diffuse, darker shadow.

We're really just trying to make it look good, so to some extent, whether it looks perfectly accurate is not necessarily the most important thing; keeping things attractive and performant may be more important.

But there's at least an interesting implementation idea here I can resist writing down.... An interesting property of "guassian convolution" shadows is that they add up properly for non-overlapping areas. If we have a shape 

                 +------------------+
                 |        A         |
       +---------+··················+-------------+
       |                  B                       |
       +------------------------------------------+
       
Then we can draw it by first drawing the shadow for A, then for B. So, using this information, if we have two overlapping windows:

  +------------------+
  |      A           |
  |    +-------------------+
  |    |                   |
  +----|        B          |
       |                   |
       +-------------------+

And we draw, bottom to top:

  - Shadow(A - B)
  - A - B
  - Shadow(B)
  - B

We get the result you want - the shadow of (A union B) is drawn on the background, and the shadow of B is drawn on A.

What we draw currently is actually:

  - Shadow(A)
  - A - B
  - Shadow(B)
  - B

We're already computing the "unobscured area" of each window, so we just have to draw the shadow of that rather than the shadow of the full window. And we even compute the "unobscured area" as a rectangle list, so it's easy to draw it's shadow using the decomposition technique of the first picture.

Practical difficulties:

 - ARGB windows - for correctness, they don't are counted as completely transparent for computing obscured areas. We could compute two sets of obscured areas - one for shadow, one for knowing what portions of windows we need to draw, but that's getting expensive. It certainly could be ignored first pass.

 - The approach to drawing correct guassian shadows we are using so far depends on the shadow being of a rectangle that is larger in both dimensions than the window size for the guassian blur (if the guassian blur has "sigma" of 8 that means a window size of about 40). One solution of this is create a set of Guassian blurs of rectangles of different sizes for example the 49 textures:

 width=[1,2,4,8,16,32,64] x width=[1,2,4,8,16,32,64]

Then any smaller rectangle can be decomposed reasonably efficiently. There may be other solutions ... I haven't looked at the literature, that's just off the top of my head.

If someone was going to fool around with this level of complexity, it's probably best to create a test program that just has a bunch of rectangles in a ClutterStage that you can move around and look at their shadows, and get that working well before doing it in Mutter.

A pedantic idea:

Right now windows which don't overlap cast shadows on each other in metacity, compiz, mutter... For example, if you are working with two windows which are 'snapped' against each with their borders, the active window will cast a shadow on the other window. This is not really nice, if you want to see the content of both windows properly at the same time.

A solution to this would be if we *somehow* could consider all windows to be in layers, where each layer would contain only windows that don't overlap and thus don't cast shadows on each other.

(A disadvantage could be that this makes it more difficult to recognize the active window.)

Whew, I haven't read all the implementation comments (I'm just a user that likes Metacity's shadows), I just want to add my vote for setting the default look to be "top light source with bigger widths, z-order dependent". 

This improves clarity/visual distinction between windows in an exponential manner, and is very good for usability IMHO. Metacity's shadows completely kill the competition in terms of enhancing contrast between overlapping windows. Another example of this (metacity is probably much inspired from it) is... Mac OS X's window manager.

Related to bug 603066 I guess.

Created attachment 172662 [details] [review]
Make shadows pretty and configurable

Here's my attempt at a shadow rewrite. What it does is:

 - Properly handles shaped shadows for shaped windows
 - Makes shadows configurable on a per-window bases and makes the
   default larger and directly below rather than to the bottom
   left.

I haven't spent much time on the defaults and I haven't really tried to do here is to handle using different shadows for different windows:

 - A bigger shadow for the active window
 - Smaller shadows for menus and other "attached" windows
 - Omitting the portion of the shadow extending above a menubar
   attached menu or a entry dropdown (needs toolkit support?)

So some things look a bit funny. I'm not completely sure how much smarts should go in Mutter and how much in the plugin - it might be easiest to put everything in the plugin, though that does mean duplicated logic between different plugins.

I didn't end up my technique described above of adding shadows together. It turned out to have problems with limited precision causing banding. If you have a shadow of a shape like a circle that has lots of thin narrow bands then you are adding together 30 or shadows at every point to get the result and jump from 1 * 30 to 2 * 30 to 3 * 30 at the edges which looks bad. I was able to work around that with Floyd-Steinberg dithering and a small radius blur pass at the end to get rid of dithering artifacts, but in the end it was somewhat slower than other aproaches I tried.

The pure-software implementation here (multiple passes of box blur limited to the border regions of the shadow) is also multiple times faster than anything I was able to do using GPU shaders (on recent Intel integrated graphics, admittedly) because it exploits scan-line coherency.

There is no attempt to suppress a double-dark shadow from overlapped windows. While the double-shadow is physically unrealistic, it's hard to get configurations where it's obviously wrong to the eye, and it would be a lot of work and a lot of performance hit compared to this approach here of just drawing the shadow.

There is still some optimization that could be done:

 - Don't draw the portion of shadows underneath windows using the clip region we already compute
 - Optimize computation of shadows of square windows as a special case
 - Remaining optimization for the box blur step (some notes in the code)

The first one may be important - the other ones not obviously so until they show up in profiling - the code here can blur a 1024x1024 cdo in 10ms on my system. (Normally we don't generate shadows larger than around ~130x130 since we can just scale them up as a 9-slice.)

Created attachment 173186 [details] [review]
Make shadows pretty and configurable

Last version had a crash handling unshaped windows that had not yet been
mapped; work things so that we don't compute the MetaWindowShape until
a window is mapped.

Created attachment 174281 [details] [review]
Make shadows pretty and configurable

Rebase to current master

Created attachment 174283 [details] [review]
Add frame type for attached modal dialogs

Add a new frame type META_FRAME_TYPE_ATTACHED which is used for
attached modal dialogs.

The theme format version is bumped to 3.2, and attached windows
can have borders defined in a metacity-theme-3.xml as:

 <window version=">= 3.2" type="attached" style_set="[name]"/>

If no style is defined for "attached", drawing will fall back
to the "border" type.

Created attachment 174284 [details] [review]
MetaShadowFactory: Add the ability to top-fade a shadow

For attached modal dialogs, we want the shadow to fade out at the top
as if the window was glued to the parent at the top. Add a
shadow-top-fade property to MetaWindowActor and the corresponding
parameter to meta_shadow_factory_get_shadow().

The internal implementation of MetaShadow is adjusted to work
in terms of an "inner border" and "outer border" instead of doing
the calculations in terms of an aggregate border and the spread
of the shadow. The old way of doing things gets clumsy when the
top_fade distance is added in as well.

Created attachment 174285 [details] [review]
MetaShadowFactory: convert to a GObject

Created attachment 174286 [details] [review]
Make MetaShadowFactory public

The basic MetaShadowFactory type is moved to a public header, while
the functions to fetch and paint shadows are kept private.
The public object will be used for configuration of shadows by
plugins.

Created attachment 174287 [details] [review]
Export meta_window_appears_focused()

Move meta_window_appears_focused() into the public window.h so
we can use it to change the shadow type.

Created attachment 174288 [details] [review]
Add meta_window_get_frame_type()

Add a public function to get the frame type for a window; the
code is refactored from existing code in core.c.

Created attachment 174289 [details] [review]
Export meta_frame_type_to_string()

Frame types will form the bases of shadow classes, which are strings,
so export the to-string function so that we can do the conversion
uniformly.

Created attachment 174290 [details] [review]
Make window shadows globally configurable

Instead of setting shadow parameters on individual windows, add the
idea of a "shadow class". Windows have default shadow classes based
on their frame and window type, which can be overriden by setting
the shadow-class property.

Each shadow class has separably configurable parameters for the
focused and unfocused state. New shadow classes can be defined with
arbitrary names.

Created attachment 174291 [details] [review]
Use a template material for shadows

To avoid unnecessary shader recompilation, use a root template material
for all shadows.

Created attachment 174292 [details] [review]
Report a correct paint volume for shadowed windows

Since we paint shadows directly now rather than using a child actor
in the ClutterGroup, we need to implement get_paint_volume() for
Clutter 1.5.

Created attachment 174293 [details] [review]
Implement more accurate clipping of obscured shadows

Instead of making optimizing obscured shadows an all-or-none operation,
pass the clip region to meta_shadow_paint() and only paint the 9-slices
that are at least partially visible.

Created attachment 174294 [details] [review]
Add meta_window_get_maximized() and meta_window_is_fullscreen()

These functions duplicate existing properties; they are added for
convenience and to avoid the GObject property code on some
performance critical painting paths.

Created attachment 174295 [details] [review]
Omit shadows for fullscreen and maximized windows

Fullscreen and maximized windows never have visible shadows - the only
case where we would ever see them is if they bleed onto an adjacent
monitor and that looks bad.

It's small performance win to avoid computing them, and this also avoids
painting the top shadow for all maximized windows in GNOME Shell - since
the top panel isn't a X window, it doesn't factor into the computation
of what parts of windows are visible and maximized windows are computed
as having a top shadow.

Some squashing of the patches is possible, but I've left it split out here, since I think work has already begun on reviewing the big technical initial patch. The add-ons are all about configuration and some optimization. I've also pushed this all to the 'configurable-shadows' branch in Git for easy testing.

The configuration from JS would look like:

 Meta.ShadowFactory.get_default().set_params("normal", true,
   { radius: 10, top_fade, 20, x_offset: 0, y_offset 5, opacity: 255 });

(true is for the focused state, top_fade = -1 for no top_fade. See comments for MetaShadowParams and meta_shadow_factory_set_params() for more details.)

I think we might as well put most of the tweaks we come up with for the shadow into Mutter by default ... I'm not sure that configuration in GNOME Shell is really necessary. But it's probably good for playing around and finding the right values.

One reason I didn't put the shadow configuration in the theme is that I felt it might take some experimentation to find the right shadow classes, so the rigidity of the theme format would be confining. And putting it in the theme didn't really fit with the idea that a plugin could come up with its own shadow classes, configure them, and set them on a window with window_actor.shadow_type = "my-shadow-class";

The default shadow classes are defined as:

MetaShadowClassInfo default_shadow_classes[] = {
  { "normal",       { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },
  { "dialog",       { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },
  { "modal_dialog", { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },
  { "utility",      { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },
  { "border",       { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },
  { "menu",         { 12, -1, 0, 8, 255 }, { 6, -1, 0, 4, 255 } },

  { "popup-menu",    { 6, -1, 0, 4, 255 }, { 6, -1, 0, 4, 255 } },

  { "dropdown-menu", { 6, 25, 0, 4, 255 }, { 6, 100, 0, 4, 255 } },
  { "attached",      { 6, 25, 0, 4, 255 }, { 6, 100, 0, 4, 255 } }
};

So they are the MetaFrameType types but with a couple of additions. I went around on whether it would be better to base them on MetaWindowType, but it wasn't clear they were a better fit, and there were a whole lot more of them :-)

Created attachment 174298 [details] [review]
MetaWindowActor: Fix crashes when mapping and unmapping windows

The assumptions made when getting the size of the window for the
paint volume turned out not to be accurate in all cases -
get_paint_volume() could be called on windows without computed
bounds.

Created attachment 174398 [details] [review]
MetaShadowFactory: Add the ability to top-fade a shadow

Fix a width/height confusion

Created attachment 174399 [details] [review]
Report a correct paint volume for shadowed windows

Initialize ClutterVertex.z

Review of attachment 174281 [details] [review]:

Sorry this took a while - embarrassing, but the main difficulty was a language problem (the mathematical meaning of "span").
All comments are minor; being able to configure shadows on the fly certainly rocks!

::: src/compositor/meta-shadow-factory.c
@@ +198,3 @@
+                                              dest_x[i + 1], dest_y[j + 1],
+                                              src_x[i], src_y[j],
+                                              src_x[i + 1], src_y[j + 1]);

Why not cogl_rectangles_with_texture_coords()?

@@ +668,3 @@
+  shadow->texture = make_shadow (region, radius);
+  shadow->material = cogl_material_new ();
+  cogl_material_set_layer (shadow->material, 0, shadow->texture);

The clutter guys would probably insist on copying a template material here :-)

::: src/compositor/meta-window-actor.c
@@ +267,3 @@
+  pspec = g_param_spec_int ("shadow-y-offset",
+                            "Shadow Y Offset",
+                            "Distance shadow is offset in the vertical direction in piyels",

Typo

@@ +692,3 @@
 
+  if (priv->shadow_radius == 0)
+    return FALSE;

Hmm, no hard shadows? Should probably be documented somewhere (or include offsets in the test)

@@ +1793,3 @@
+    {
+      old_shadow = priv->shadow;
+      priv->shadow = NULL;

Out of curiosity: Why do you defer the call to meta_shadow_unref()?

::: src/compositor/meta-window-shape.c
@@ +143,3 @@
+      g_print ("%d: +%d+%dx%dx%d => +%d+%dx%dx%d\n",
+               i, iter.rectangle.x, iter.rectangle.y, iter.rectangle.width, iter.rectangle.height,
+               shape->rectangles[i].x, shape->rectangles[i].y, shape->rectangles[i].width, shape->rectangles[i].height);

Code is commented out, so it's not really a problem but: i should be iter.i

::: src/compositor/meta-window-shape.h
@@ +35,3 @@
+ * if you the regions representing two windows that around rounded rectangles,
+ * with the same corner regions, but different sizes, they have the
+ * same MetaWindowShape.

I have some trouble parsing that sentence - there's at least a verb missing, right?

::: src/compositor/region-utils.c
@@ +48,3 @@
+typedef struct
+{
+  /* To merge regions in a binary tree, we need to keep track of The way these are filled is in the pattern:

"keep track of the way these are filled as in the pattern"?

@@ +203,3 @@
+    meta_region_builder_add_rectangle (builder,
+                                         y - y_amount, x - x_amount,
+                                         height + 2 * y_amount, width + 2 * x_amount);

Indentation.

@@ +207,3 @@
+    meta_region_builder_add_rectangle (builder,
+                                         x - x_amount, y - y_amount,
+                                         width + 2 * x_amount, height + 2 * y_amount);

Dito.

Review of attachment 174283 [details] [review]:

Looks good, but doc/theme-format.txt should be updated.

Review of attachment 174285 [details] [review]:

Looks good.

Review of attachment 174286 [details] [review]:

Just a minor comment below:

::: src/include/meta-shadow-factory.h
@@ +48,3 @@
+GType meta_shadow_factory_get_type (void);
+
+MetaShadowFactory *meta_shadow_factory_new        (void);

If we expect plugins to use the singleton factory returned from meta_factory_get_default(), shouldn't this be moved to the private header file?

Review of attachment 174287 [details] [review]:

Looks good.

Review of attachment 174288 [details] [review]:

Looks good.

Review of attachment 174289 [details] [review]:

Yup.

Review of attachment 174398 [details] [review]:

Looks good.

::: src/compositor/meta-shadow-factory.c
@@ +615,3 @@
+  /* We offset the passed in pixels to crop off the extra area we allocated at the top
+   * in the case of top_fade >= 0. We also account for padding at the left for symmetry
+   * though that doesn't currnetly occur.

Typo: currently

Review of attachment 174290 [details] [review]:

Looks good.

::: src/compositor/meta-window-actor.c
@@ +39,3 @@
+   * don't immediately unreference the old shadow, we just flag it as
+   * dirty and recompute it when we next need it (recompute_focused_shadow,
+   * recompute_unfocused_shadow.)  Because of the our extraction of

s/the//

Review of attachment 174291 [details] [review]:

Scrap my comment from a previous review - looks good.

Review of attachment 174293 [details] [review]:

Looks good.

Review of attachment 174294 [details] [review]:

Looks good.

Review of attachment 174295 [details] [review]:

Looks good.

Review of attachment 174399 [details] [review]:

Looks good.

Review of attachment 174298 [details] [review]:

Looks good, though it might be worth squashing with the paint_volume patch.

(In reply to comment #40)
> Review of attachment 174281 [details] [review]:
> 
> Sorry this took a while - embarrassing, but the main difficulty was a language
> problem (the mathematical meaning of "span").
> All comments are minor; being able to configure shadows on the fly certainly
> rocks!
> 
> ::: src/compositor/meta-shadow-factory.c
> @@ +198,3 @@
> +                                              dest_x[i + 1], dest_y[j + 1],
> +                                              src_x[i], src_y[j],
> +                                              src_x[i + 1], src_y[j + 1]);
> 
> Why not cogl_rectangles_with_texture_coords()?

Offhand, I don't think it's a significant win - because of the Cogl journal, the main difference is that we save a little bit of validation work per rectangle.... so in general doesn't seem worth having to manage a temporary array of float rectangles. I could be wrong ... hard to know without writing some benchmarks.

> @@ +668,3 @@
> +  shadow->texture = make_shadow (region, radius);
> +  shadow->material = cogl_material_new ();
> +  cogl_material_set_layer (shadow->material, 0, shadow->texture);
> 
> The clutter guys would probably insist on copying a template material here :-)

Done in a later patch.

> ::: src/compositor/meta-window-actor.c
> @@ +267,3 @@
> +  pspec = g_param_spec_int ("shadow-y-offset",
> +                            "Shadow Y Offset",
> +                            "Distance shadow is offset in the vertical
> direction in piyels",
> 
> Typo

Over-agressive x => y replacement. :-) Param spec and its blurb is gone in a later patch.

> @@ +692,3 @@
> 
> +  if (priv->shadow_radius == 0)
> +    return FALSE;
> 
> Hmm, no hard shadows? Should probably be documented somewhere (or include
> offsets in the test)

Later changed it so that radius 0 shadows are hard. I don't try to optimize it and skip the blur passes so it's a bit slow, but it's just a optimization.

> @@ +1793,3 @@
> +    {
> +      old_shadow = priv->shadow;
> +      priv->shadow = NULL;
> 
> Out of curiosity: Why do you defer the call to meta_shadow_unref()?

Later patch adds a comment that sort of explains it:

 /* MetaShadowFactory only caches shadows that are actually in use;
  * to avoid unnecessary recomputation we do two things: 1) we store
  * both a focused and unfocused shadow for the window. If the window
  * doesn't have different focused and unfocused shadow parameters,
  * these will be the same. 2) when the shadow potentially changes we
  * don't immediately unreference the old shadow, we just flag it as
  * dirty and recompute it when we next need it (recompute_focused_shadow,
  * recompute_unfocused_shadow.)  Because of the our extraction of
  * size-invariant window shape, we'll often find that the new shadow
  * is the same as the old shadow.
  */

If the old and new shadows are the same, doing the deferred unref prevents the old shadow being removed from the cache before it can be reused for the new shadow.


> ::: src/compositor/meta-window-shape.c
> @@ +143,3 @@
> +      g_print ("%d: +%d+%dx%dx%d => +%d+%dx%dx%d\n",
> +               i, iter.rectangle.x, iter.rectangle.y, iter.rectangle.width,
> iter.rectangle.height,
> +               shape->rectangles[i].x, shape->rectangles[i].y,
> shape->rectangles[i].width, shape->rectangles[i].height);
> 
> Code is commented out, so it's not really a problem but: i should be iter.i

fixed.

> ::: src/compositor/meta-window-shape.h
> @@ +35,3 @@
> + * if you the regions representing two windows that around rounded rectangles,
> + * with the same corner regions, but different sizes, they have the
> + * same MetaWindowShape.
> 
> I have some trouble parsing that sentence - there's at least a verb missing,
> right?

Pretty mangled, fixed it to:

For example, the regions representing two windows that are rounded rectangles,  with the same corner radius but different sizes, have the same MetaWindowShape

> ::: src/compositor/region-utils.c
> @@ +48,3 @@
> +typedef struct
> +{
> +  /* To merge regions in a binary tree, we need to keep track of The way these
> are filled is in the pattern:
> 
> "keep track of the way these are filled as in the pattern"?

Looks like a bunch of missing text, changed to:

To merge regions in binary tree order, we need to keep track ofthe regions that we've already merged together at different levels of the tree. We fill in an array in the pattern:      
 
> @@ +203,3 @@
> +    meta_region_builder_add_rectangle (builder,
> +                                         y - y_amount, x - x_amount,
> +                                         height + 2 * y_amount, width + 2 *
> x_amount);
> 
> Indentation.
> 
> @@ +207,3 @@
> +    meta_region_builder_add_rectangle (builder,
> +                                         x - x_amount, y - y_amount,
> +                                         width + 2 * x_amount, height + 2 *
> y_amount);
> 
> Dito.

Fixed.

(In reply to comment #43)
> Review of attachment 174286 [details] [review]:
> 
> Just a minor comment below:
> 
> ::: src/include/meta-shadow-factory.h
> @@ +48,3 @@
> +GType meta_shadow_factory_get_type (void);
> +
> +MetaShadowFactory *meta_shadow_factory_new        (void);
> 
> If we expect plugins to use the singleton factory returned from
> meta_factory_get_default(), shouldn't this be moved to the private header file?

Yeah, probably. There's absolutely nothing you can do with your own shadow factory since we don't expose the shadow code.

(In reply to comment #41)
> Review of attachment 174283 [details] [review]:
> 
> Looks good, but doc/theme-format.txt should be updated.

Done.

(In reply to comment #47)
> Review of attachment 174398 [details] [review]:
> 
> Looks good.
> 
> ::: src/compositor/meta-shadow-factory.c
> @@ +615,3 @@
> +  /* We offset the passed in pixels to crop off the extra area we allocated at
> the top
> +   * in the case of top_fade >= 0. We also account for padding at the left for
> symmetry
> +   * though that doesn't currnetly occur.
> 
> Typo: currently

Fixed.

(In reply to comment #48)
> Review of attachment 174290 [details] [review]:
> 
> Looks good.
> 
> ::: src/compositor/meta-window-actor.c
> @@ +39,3 @@
> +   * don't immediately unreference the old shadow, we just flag it as
> +   * dirty and recompute it when we next need it (recompute_focused_shadow,
> +   * recompute_unfocused_shadow.)  Because of the our extraction of
> 
> s/the//

Fixed.

Review of attachment 174298 [details] [review]:

Squashed.

Patch pushed. Thanks for the reviews, Florian! Closing bug, any further tweaking
we need to what shadows parameters we want and what windows get shadowed will
be handled separately.

Attachment 174281 [details] pushed as 9f4942e - Make shadows pretty and configurable
Attachment 174283 [details] pushed as ed2fbcd - Add frame type for attached modal dialogs
Attachment 174285 [details] pushed as d56d267 - MetaShadowFactory: convert to a GObject
Attachment 174286 [details] pushed as 3c4d527 - Make MetaShadowFactory public
Attachment 174287 [details] pushed as 6b16604 - Export meta_window_appears_focused()
Attachment 174288 [details] pushed as 52aebdf - Add meta_window_get_frame_type()
Attachment 174289 [details] pushed as 7952feb - Export meta_frame_type_to_string()
Attachment 174290 [details] pushed as 1bbaec8 - Make window shadows globally configurable
Attachment 174291 [details] pushed as 21a246e - Use a template material for shadows
Attachment 174293 [details] pushed as ca5f2ac - Implement more accurate clipping of obscured shadows
Attachment 174294 [details] pushed as d4c28fc - Add meta_window_get_maximized() and meta_window_is_fullscreen()
Attachment 174295 [details] pushed as 8b24711 - Omit shadows for fullscreen and maximized windows
Attachment 174398 [details] pushed as 8eb3194 - MetaShadowFactory: Add the ability to top-fade a shadow
Attachment 174399 [details] pushed as 15f9590 - Report a correct paint volume for shadowed windows