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 603691 - Add drop shadows to close buttons in overview
Add drop shadows to close buttons in overview
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-12-03 12:10 UTC by Florian Müllner
Modified: 2010-01-05 20:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implement drop shadows for StWidget (27.90 KB, patch)
2009-12-03 12:10 UTC, Florian Müllner
none Details | Review
[Overview] Add drop shadows to close buttons (735 bytes, patch)
2009-12-03 12:15 UTC, Florian Müllner
reviewed Details | Review
Implement box-shadow for StWidget (27.77 KB, patch)
2009-12-17 17:26 UTC, Florian Müllner
needs-work Details | Review
[Overview] Add drop shadows to close buttons (763 bytes, patch)
2009-12-17 17:29 UTC, Florian Müllner
needs-work Details | Review
Implement -st-shadow for StWidget (23.76 KB, patch)
2009-12-20 02:34 UTC, Florian Müllner
none Details | Review
[Overview] Add drop shadows to close buttons (760 bytes, patch)
2009-12-20 02:35 UTC, Florian Müllner
none Details | Review
Implement -st-shadow for StWidget (23.66 KB, patch)
2009-12-21 16:37 UTC, Florian Müllner
needs-work Details | Review
[Overview] Add drop shadows to close buttons (759 bytes, patch)
2009-12-21 16:38 UTC, Florian Müllner
committed Details | Review
Implement -st-shadow for StWidget (24.37 KB, patch)
2009-12-22 04:29 UTC, Florian Müllner
none Details | Review
Implement -st-shadow for StWidget (24.63 KB, patch)
2009-12-30 12:37 UTC, Florian Müllner
reviewed Details | Review
Implement -st-shadow for StWidget (24.59 KB, patch)
2010-01-04 21:12 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2009-12-03 12:10:25 UTC
In the latest mockups, the close buttons have soft drop shadows.
This is an implementation on top of StWidget, modelled liberally after the CSS3 box-shadow property.

For a start, only the background-image property is affected, and there is no caching of the generated shadow textures.
Comment 1 Florian Müllner 2009-12-03 12:10:34 UTC
Created attachment 149003 [details] [review]
Implement drop shadows for StWidget

Add a CSS property for drop shadows (-shell-drop-shadow),
based on the CSS3 box-shadow property:

http://www.css3.info/preview/box-shadow/

It defers from the specification as follows:

 * the shape is determined by the widget's content,
   not its bounding box
 * no multiple shadows
 * the optional color argument may be placed anywhere

Currently, only for background images a shadow is drawn,
which is enough to add shadows to the close buttons in the
overview; it may be extended to more properties though,
if desired.
Comment 2 Florian Müllner 2009-12-03 12:15:25 UTC
Created attachment 149004 [details] [review]
[Overview] Add drop shadows to close buttons
Comment 3 Colin Walters 2009-12-17 04:35:06 UTC
Review of attachment 149003 [details] [review]:

I'll have to get to the key st-shadow-texture.c bit tomorrow; some comments follow for other parts.

::: src/st/st-drop-shadow.c
@@ +2,3 @@
+#include "st-drop-shadow.h"
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

This could use a short block comment; doesn't have to be very long; just a sentence or two.  "This object represents parameters for a drop shadow; see http://www.css3.info/preview/box-shadow/"?

@@ +5,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

While using a full GObject is fine, did you consider making this a simple public boxed structure?  Like ClutterColor?

@@ +7,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

Just allocating the color as part of the StDropShadow struct seems simpler to me.

@@ +28,3 @@
+      clutter_color_free (shadow->color);
+      shadow->color = NULL;
+    }

Then you don't need to free here.

@@ +56,3 @@
+  shadow = g_object_new (ST_TYPE_DROP_SHADOW, NULL);
+
+  shadow->color   = clutter_color_copy (color);

And then this would be:

shadow->color = *color;

@@ +69,3 @@
+  g_return_val_if_fail (ST_IS_DROP_SHADOW (shadow), NULL);
+
+  return shadow->color;

I'm pretty sure for boxeds we're going to assume (transfer full), i.e. that a copy has been made.  If you declare the return value "const" that should work.

@@ +75,3 @@
+st_drop_shadow_get_offsets (StDropShadow *shadow,
+                            gdouble      *xoffset,
+                            gdouble      *yoffset)

Missing (out) annotation.

::: src/st/st-shadow-texture.c
@@ +7,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

Could likewise allocate color as part of the struct here.

@@ +71,3 @@
+    return FALSE;
+
+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);

Duplicated.

::: src/st/st-theme-node.c
@@ +2148,3 @@
+      CRDeclaration *decl = node->properties[i];
+
+      if (strcmp (decl->property->stryng->str, "-shell-drop-shadow") == 0)

I think we should reserve the -shell prefix for things which are actually implemented in src/*.[ch] or in JS.  

We could just call this box-shadow and note deviations from the CSS3 proposal.  We already are not-quite-CSS, so I don't see a problem with another property that's not-quite-CSS, especially given that the CSS is still not standardized.
Comment 4 Colin Walters 2009-12-17 04:35:08 UTC
Review of attachment 149003 [details] [review]:

I'll have to get to the key st-shadow-texture.c bit tomorrow; some comments follow for other parts.

::: src/st/st-drop-shadow.c
@@ +2,3 @@
+#include "st-drop-shadow.h"
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

This could use a short block comment; doesn't have to be very long; just a sentence or two.  "This object represents parameters for a drop shadow; see http://www.css3.info/preview/box-shadow/"?

@@ +5,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

While using a full GObject is fine, did you consider making this a simple public boxed structure?  Like ClutterColor?

@@ +7,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

Just allocating the color as part of the StDropShadow struct seems simpler to me.

@@ +28,3 @@
+      clutter_color_free (shadow->color);
+      shadow->color = NULL;
+    }

Then you don't need to free here.

@@ +56,3 @@
+  shadow = g_object_new (ST_TYPE_DROP_SHADOW, NULL);
+
+  shadow->color   = clutter_color_copy (color);

And then this would be:

shadow->color = *color;

@@ +69,3 @@
+  g_return_val_if_fail (ST_IS_DROP_SHADOW (shadow), NULL);
+
+  return shadow->color;

I'm pretty sure for boxeds we're going to assume (transfer full), i.e. that a copy has been made.  If you declare the return value "const" that should work.

@@ +75,3 @@
+st_drop_shadow_get_offsets (StDropShadow *shadow,
+                            gdouble      *xoffset,
+                            gdouble      *yoffset)

Missing (out) annotation.

::: src/st/st-shadow-texture.c
@@ +7,3 @@
+
+
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

Could likewise allocate color as part of the struct here.

@@ +71,3 @@
+    return FALSE;
+
+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);

Duplicated.

::: src/st/st-theme-node.c
@@ +2148,3 @@
+      CRDeclaration *decl = node->properties[i];
+
+      if (strcmp (decl->property->stryng->str, "-shell-drop-shadow") == 0)

I think we should reserve the -shell prefix for things which are actually implemented in src/*.[ch] or in JS.  

We could just call this box-shadow and note deviations from the CSS3 proposal.  We already are not-quite-CSS, so I don't see a problem with another property that's not-quite-CSS, especially given that the CSS is still not standardized.
Comment 5 Colin Walters 2009-12-17 04:36:37 UTC
Review of attachment 149004 [details] [review]:

Looks fine obviously =)  (Though don't forget to update this patch if you agree on the renaming of -shell-drop-shadow)
Comment 6 Florian Müllner 2009-12-17 17:26:55 UTC
Created attachment 149924 [details] [review]
Implement box-shadow for StWidget

* rename -shell-drop-shadow to box-shadow
 * implement StBoxShadow as boxed type instead of an object
 * store ClutterColor instead of ClutterColor * in structs
 * add brief documentation to public types/methods
Comment 7 Florian Müllner 2009-12-17 17:29:09 UTC
Created attachment 149925 [details] [review]
[Overview] Add drop shadows to close buttons

Rename -shell-drop-shadow property to box-shadow
Comment 8 Owen Taylor 2009-12-18 21:36:53 UTC
Review of attachment 149924 [details] [review]:

Thanks for working on this! This approach looks like it works. At some point, I want to change things around so that almost all the painting for st-theme-node (background, borders, shadow, etc.) occurs in shell-theme-node.c with a single shell_theme_node_paint(), but this will work for now.

I don't like using the name box-shadow:

 A) box-shadow was literally a shadow of the box border, and not of the contents of the box
 B) box-shadow has been droppped from the latest borders and backgrounds module

See:

 http://old.nabble.com/-CSSWG--Minutes-and-Resolutions-2009-09-02-td25372210.html

For the discussion that apparently led to it being dropped - it sounds like the plan may be to have a more general 'drop-shadow' property that would allow shadowing contents. I'd suggest using the name '-st-shadow' for now, since what's going on with CSS seems up in the air.

I'd also like to see the documentation of the differences from CSS in the code, not in just in the commit message. Note that "the shade is determined by the widget's content" that you have in the commit message isn't quite right - as I read it, you are determining the shape by the widget's background image, not the content.

::: src/st/st-box-shadow.c
@@ +45,3 @@
+/**
+ * st_box_shadow_copy:
+ * @shadow: a #StBoxShadow

Since you are (unusually, we don't normally do that) supporting passing NULL in, you should add ", or %NULL"

@@ +55,3 @@
+st_box_shadow_copy (const StBoxShadow *shadow)
+{
+  if (G_LIKELY (shadow != NULL))

Don't use G_LIKELY/G_UNLIKELY like this. If st_box_shadow_copy() is legal to call on NULL at all, then there might be cases where it frequently gets called on NULL. If you don't deterministically know the direction of prediction with a high degree of certainty, don't use G_LIKELY at all, because using G_LIKELY can actually force a misprediction where the processor would normally get it right. (The processor will predict based on history.)

An appropriat use of G_UNLIKELY is inside the implementation of g_return_if_fail(), since g_return_if_fail() assertions *never* fail if the program is working correctly.

::: src/st/st-shadow-texture.c
@@ +35,3 @@
+st_shadow_texture_set_from_actor (StShadowTexture  *st,
+                                  ClutterActor     *actor,
+                                  GError          **error)

Separating this out from create_shadow() doesnt' really make sense to me ... you are jumping through hoops here to read data from the source actor, then store it back into the texture for this actor, then read it back out again, then convolve it, then store it back in again.

I think what you want to do is more straightforward, just in the create_shadow()

 - Get the data from the source texture
 - Create a memory buffer for the destination image
 - Do the vertical convolution pass, reading from the data  that you read and writing to destination image
 - Do the horizontal convolution pass by copying a full line into a temporary buffer, then convolving from there

The reason to do the vertical first then the horizontal is that you can just memcpy a line into a temporary buffer for the horizontal; using the temporary buffer here may seem less efficient than the approach you have of convolving between two buffers, but it actually is pretty efficient, since the real memory access penalty is getting the data into the processors L1 cache; copying data into a small buffer than reading from that buffer again is close to free.

[ Side note that might be fun to think about, if you were optimizing for speed on large images, then you might want to do the vertical convolution in horizontal slices - first convolve the first 256 lines, then the second 256 lines, and so forth. Why? ]

@@ +82,3 @@
+
+  pixels = g_malloc0 (rowstride * height);
+  cogl_texture_get_data (texture, st->format, rowstride, pixels);

One thing that its important to realize is that getting data from OpenGL (and COGL) is always a copy and you can pick the format you want. So, you don't actually need to worry about different formats - you can just pick one. Probably here you can use COGL_PIXEL_FORMAT_A_8 to just extract the alpha channel from the source texture. You can also use an A_8 format texture for the blurred shadow image and then you don't have to worry about bpp at all. To handle drawing the A_8 format texture with the right color, you'll need to create your own cogl_material.

@@ +102,3 @@
+
+static gdouble *
+calculate_gauss_kernel (gdouble sigma, guint n_values)

Would read better if it was 'calculate_guassian_kernel'

@@ +113,3 @@
+  sum = 0.0;
+
+  factor      = 1.0 / (sqrt (2 * M_PI) * sigma);

This is a normalization factor, which would be the appropriate value if you had an "unwindowed" guassian - you didn't chop it off to some width. Since you are truncating at some width, then this won't be exact, so you need to normalize yourself - which you do. Once you have that other normalization, you don't need this one.

@@ +120,3 @@
+    {
+      if ((int) sigma)
+        ret [i] = factor * exp (-(i - half) * (i - half) / exp_divisor);

You have 'array [index]' in various places instead of 'array[index]' - unlike functions, there is no space before the '['

@@ +122,3 @@
+        ret [i] = factor * exp (-(i - half) * (i - half) / exp_divisor);
+      else
+        ret [i] = 1.0;

I think it would be better to special case the hard shadow rather than convolving with an array with one element.

@@ +156,3 @@
+
+  n_channels  = st->rowstride / st->width;
+  n_values = (guint) 2 * st->sigma + 1;

This is a very narrow window - your guassian kernel looks something like:

 .-.
'   ' 

Rather than:

  .-.
_/   \_

A standard value to use for the width of the window would be 5 * sigma - 2.5 on either side.

::: src/st/st-theme-node.c
@@ +2150,3 @@
+      if (strcmp (decl->property->stryng->str, "box-shadow") == 0)
+        {
+          /* Set value for width/color/node in any order */

what's 'node' here?

@@ +2159,3 @@
+
+          /* default to foreground color if not specified otherwise */
+          st_theme_node_get_foreground_color (node, &color);

The old box-shadow property had "omitted colors are a UA-chosen color" - defaulting to black seems to be more appropriate than defaulting to the foreground color, which if it isn't black, is likely an inappropriate color for a shadow - e.g., in the shell we generally use white for the foreground color, but there are no white shadows!

::: src/st/st-widget.c
@@ -297,4 +310,4 @@
-        0,
-        0,
-        box->x2 - box->x1,
-        box->y2 - box->y1
+        priv->offset_x,
+        priv->offset_y,
+        priv->offset_x + box->x2 - box->x1,
+        priv->offset_y + box->y2 - box->y1,

I don't think a shadow should ever move the contents of the actor around - if the shadow is to the upper left of the actor it should just be allocated outside the allocation of the actor to the upper left. (There is no requirement in Clutter that the children of an actor have to be confined within the bounds of the actor.)

@@ +953,3 @@
+                                   box_shadow->blur);
+
+      if (priv->background_image_shadow)

This if can be combined with the previous if
Comment 9 Owen Taylor 2009-12-18 21:37:35 UTC
Review of attachment 149925 [details] [review]:

Marking needs-work to correspond to my comment about the box-shadow name.
Comment 10 Colin Walters 2009-12-18 22:11:32 UTC
(In reply to comment #8)
> 
> ::: src/st/st-box-shadow.c
> @@ +45,3 @@
> +/**
> + * st_box_shadow_copy:
> + * @shadow: a #StBoxShadow
> 
> Since you are (unusually, we don't normally do that) supporting passing NULL
> in, you should add ", or %NULL"

Note we have an annotation for this: (allow-none)
Comment 11 Florian Müllner 2009-12-20 02:34:50 UTC
Created attachment 150089 [details] [review]
Implement -st-shadow for StWidget

Updated patch according to Owen's review.
Comment 12 Florian Müllner 2009-12-20 02:35:33 UTC
Created attachment 150090 [details] [review]
[Overview] Add drop shadows to close buttons

Use -st-shadow instead of box-shadow.
Comment 13 Florian Müllner 2009-12-21 16:37:53 UTC
Created attachment 150183 [details] [review]
Implement -st-shadow for StWidget

Update patch to assume blur_radius == blur_window_size instead of
blur_radius == sigma.
Comment 14 Florian Müllner 2009-12-21 16:38:14 UTC
Created attachment 150184 [details] [review]
[Overview] Add drop shadows to close buttons

Adjust the shadow values a little
Comment 15 Owen Taylor 2009-12-21 19:50:57 UTC
Review of attachment 150183 [details] [review]:

Nice job implementing my suggestions!

Most of the remaining comments are stylistic.

::: src/st/st-shadow-texture.c
@@ +61,3 @@
+st_shadow_texture_create_shadow (StShadowTexture *st,
+                                 ClutterActor *actor,
+                                 GError **error)

Function parameters need alignment

I don't like using a GError to propagate back error messages one level where you then log them - it's better to just log them directly. GError should be used a) if an error message will be displayed to a human in a dialog, etc b) if the caller might do something differently depending on the exact error code.

@@ +85,3 @@
+  height_in = cogl_texture_get_height (texture);
+
+  pixels_in  = g_malloc0 (width_in * height_in);

It's slightly bad house-keeping to use unaligned lines; I doubt it matters here, but some code might fall back to a slow path for this. I'd probably do:

 in_rowstride = (pixels + 3) * ~3; /* pad to a multiple of 4 */

Probably the bigger advantage of doing this is having an explicit rowstride variable, which will make expressions that multiply by or use the rowstride more obvious. Same for the out buffer.

@@ +102,3 @@
+      gint     x, y, i;
+
+      blur_window = (gint) st->blur_radius;

"window" is a confusing term here, since by "window" we'd generally mean the full width of the kernel - that is n_values.

@@ +106,3 @@
+
+      width_out  = width_in  + n_values;
+      height_out = height_in + n_values;

The blurred image seems to be expanded by n_values - 1 not n_values - that is, by n_values / 2 on either side. Calling this something else, say 'half_width' would be less confusing.

@@ +115,3 @@
+      for (x = 0; x < width_in; x++)
+        for (y = 0; y < height_out; y++)
+          {

Hmm, I found this pretty confusing and hard to decipher (though correct). Here's an attempt to rewrite it in a way that makes more sense to me; not that it is perfectly self-explanatory either.

 y_in = y_out - half_width;

 /* We read from the source at 'y = y_in + i - half_width'; 
  * clamp the full i range [0,n_values) so that y is in [0, height_in). 
  */
 i0 = MAX(half_width - y_in, 0);
 i1 = MIN(height_in + half_width - y_in, n_values);

 in_pixel = in_pixels + (y_in + i0 - half_width) * in_rowstride + x_in;
 out_pixel = out_pixels + y_out * out_rowstride + (x_in + half_width);

 for (i = i0; i < i1; i++) {
     *out_pixel += *in_pixel * kernel[i];
     in_pixel += rowstride;
 }

So, differences:

 - Explicit x_in/y_in/y_out to avoid the confusing part of your loop where you have x being in in coordinates and y in out coordinates
 - Use of y_in to avoid the problem where the '2 * blur_window' factor comes from two separate sources - the offset in the kernel and the offset between the input and output coordinates systems.
 - Use of pointer arithmetic to avoid [index] (not an efficiency thing, but for clarity)
 - Using of i0/i1 to move the bounds checks out of the inner loop (an effiency thing)

@@ +131,3 @@
+      for (y = 0; y < height_out; y++)
+        {
+            guchar *line = g_memdup (pixels_out + y * width_out, width_out);

I think it's better to use a fixed buffer and avoid allocating one for each row

@@ +161,3 @@
+                                        pixels_out);
+
+  cogl_material_set_layer_combine_constant (material, 0, &st->color);

I think iut would be just slightly nicer to use cogl_material_set_color(), and a combine of 'RGBA = MODULATE (PREVIOUS, TEXTURE[A])'

@@ +171,3 @@
+  if (success)
+    {
+      clutter_texture_set_cogl_material (CLUTTER_TEXTURE (st), material);

Since you don't have a fallback, there's no point in checking success - you might as well go ahead and set the material anyways (the only failure that would make sense would be something wrong with the combine string); in fact, you can just pass in NULL for error, and COGL will go ahead and warn about any blend compilation failures, which is all you could do anyways.

@@ +225,3 @@
+                             color->blue, color->alpha);
+  st->blur_radius = blur;
+  st->sigma = .2 * blur + .5;

This factor seems very small to me.

For comparison I looked at the Firefox code - for box-shadow and text-shadow the blur radius is related to their implementation in terms of successive box blurs. The ComputeBlurRadius() function http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxBlur.cpp#28 is used to figure out the blur radius for a given sigma when doing SVG blurs (which are defined in terms of stdDeviation/sigma), and ends up working out approximately to blur_radius = sigma * 1.9.

::: src/st/st-shadow-texture.h
@@ +22,3 @@
+                                                 ClutterColor    *color,
+                                                 gdouble          blur_radius);
+gdouble       st_shadow_texture_get_blur_radius (StShadowTexture *shadow);

I think it would be better to reduce the leakage out of the ShadowTexture class by doing something like:

 void st_shadow_texture_adjust_allocation (StShadowTexture *shadow,
                                           ClutterActorBox *allocation);

That is - let the shadow texture itself figure out how much extra space it needs to be allocated.

::: src/st/st-theme-node.c
@@ +2128,3 @@
+ * Gets the value for the -st-shadow style property
+ *
+ * Return value: (transfer none): the box shadow, or %NULL

"the box shadow" should be "the shadow object" or something.

::: src/st/st-widget.c
@@ +376,3 @@
+            /* To avoid sharp edges of the blurred texture, the shadow texture
+               is larger than the original texture - on each side,
+               blur_window_size pixels are added, which we have to consider

I don't think this has much about "avoid sharp edges" - it's just that when we blur an image it gets bigger.

As discussed in IRC, there's a general problem here when the image is scaled; we are blurring the unscaled image and then scaling it up or down, which will change the effective blur radius.

I don't have any good solution for this, so let's just document the problem and ignore it for now.

The long term approach here is:

 - As noted in previous comment, switch all the CSS painting into a shell_theme_node_paint()
 - When shadowing, paint the CSS stuff (borders, background, etc.) into a Cogl offscreen buffer
 - Blur that (using shaders when possible, otherwise by fetching the data back to the CPU)

@@ +938,3 @@
+   * parameter - its interpretation defers significantly in that the shadow's
+   * shape is not determined by the bounding box, but the element's other
+   * CSS properties.

"but by the CSS background image (we could extend this in the future to take other CSS properties like border and background color into account.)"
Comment 16 Florian Müllner 2009-12-22 04:19:15 UTC
(In reply to comment #15)
> Review of attachment 150183 [details] [review]:
> 
> Nice job implementing my suggestions!

Thanks! I updated the patch according to the last review, with one notable exception:

> 
> @@ +161,3 @@
> +                                        pixels_out);
> +
> +  cogl_material_set_layer_combine_constant (material, 0, &st->color);
> 
> I think iut would be just slightly nicer to use cogl_material_set_color(), and
> a combine of 'RGBA = MODULATE (PREVIOUS, TEXTURE[A])'
 
My first try was along the lines of

   cogl_material_set_color (material, &st->color);
   cogl_material_set_layer (material, 0, texture);
   cogl_material_set_layer_combine (material, 0,
                                   "RGBA = MODULATE (PRIMARY, TEXTURE[A])",
                                   &error);

This is more or less what you are suggesting, right? Unfortunately, that's giving me white shadows (replacing PRIMARY with PREVIOUS does not change that).
For testing purposes, I tried a combine string of "RGBA = REPLACE (PRIMARY)",
which results in a plain white rectangle - again, not what I would expect.

Probably I'm missing something dead simple here, I'll give it some more thought during the holidays ...
Comment 17 Florian Müllner 2009-12-22 04:29:36 UTC
Created attachment 150217 [details] [review]
Implement -st-shadow for StWidget

Updated patch - see comment above what's left out from the review.

As I don't know if/when I'll have internet during this week, I'll take this
opportunity to wish all of you a merry christmas!
Comment 18 Owen Taylor 2009-12-22 15:31:35 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > Review of attachment 150183 [details] [review] [details]:
> > 
> > Nice job implementing my suggestions!
> 
> Thanks! I updated the patch according to the last review, with one notable
> exception:
> 
> > 
> > @@ +161,3 @@
> > +                                        pixels_out);
> > +
> > +  cogl_material_set_layer_combine_constant (material, 0, &st->color);
> > 
> > I think iut would be just slightly nicer to use cogl_material_set_color(), and
> > a combine of 'RGBA = MODULATE (PREVIOUS, TEXTURE[A])'
> 
> My first try was along the lines of
> 
>    cogl_material_set_color (material, &st->color);
>    cogl_material_set_layer (material, 0, texture);
>    cogl_material_set_layer_combine (material, 0,
>                                    "RGBA = MODULATE (PRIMARY, TEXTURE[A])",
>                                    &error);
> 
> This is more or less what you are suggesting, right? Unfortunately, that's
> giving me white shadows (replacing PRIMARY with PREVIOUS does not change that).

Answer to the mystery lies in clutter_texture_paint():

  cogl_material_set_color4ub (priv->material,
                              paint_opacity, paint_opacity, paint_opacity, paint_opacity);

could override paint(), but let's just go with your current solution.

The only other comments I had on the current version are:

 - I'd like to see a three way split:

    blur_radius
    sigma = blur_radius / 1.9 (or * 0.5, doesn't really matter; 
            would be good to have a comment however that contains 
            the information I provided above that relates the blur
            sigma relationship to what Firefox uses)            
    window =  sigma * 5 (== n_values)
 
   Instead of your equating window with sigma. Visually, the effect
   looks fine as is - I doubt that using a slightly larger window
   will make much of a difference, but the window size is really an 
   independent parameter and it's confusing to tie it into the blur 
   radius.

 - You neeed to premultiply the color value - it doesn't matter for
   black, but would for other colors. So, compared to your current
   version:

   if (color)
-    cogl_color_set_from_4ub (&st->color,
-                             color->red,  color->green,
-                             color->blue, color->alpha);
+    {
+      cogl_color_set_from_4ub (&st->color,
+                               color->red,  color->green,
+                               color->blue, color->alpha);
+      cogl_color_premultiply (&st->color);
+    }

Otherwise the patches look good to commit to me. (And I tried it out and the effect with the shadowed close buttons looks nice.)
Comment 19 Florian Müllner 2009-12-30 12:37:41 UTC
Created attachment 150574 [details] [review]
Implement -st-shadow for StWidget

Updated patch to
 * premultiply the color value
 * distinguish more cleanly between sigma / blur radius / blur window
Comment 20 Owen Taylor 2010-01-04 19:58:30 UTC
Review of attachment 150574 [details] [review]:

Generally looks very good, found a few more minor things to complain about :-)

::: src/st/st-shadow-texture.c
@@ +104,3 @@
+      gint     x_in, y_in, x_out, y_out, i;
+
+      n_values = 5 * (gint) st->sigma;

Probably better as (gint) 5 * st->sigma

@@ +185,3 @@
+
+  success =
+    cogl_material_set_layer_combine (material, 0,

This can't fail in any normal case, so I'd omit checking the return value - if it fails, it will warn, and misbehave - that's as good as if st_shadow_texture_new() fails.

@@ +186,3 @@
+  success =
+    cogl_material_set_layer_combine (material, 0,
+                                     "RGBA = MODULATE (CONSTANT, TEXTURE[A])",

I think a comment above here along the lines of:

 /* We ignore the material color, which encodes the overall opacity of the
  * actor, so setting an ancestor of the shadow to partially opaque won't
  * work. The easiest way to fix this would be to override paint(). */

would be a good idea

@@ +231,3 @@
+ * defaults to fully opaque black.
+ *
+ * Returns: a new #ClutterActor holding a shadow texture for @actor

If you actually returned NULL on failure (as you do now), that should be documented. I don't think it's worth the code or the docs since it can't happen except for bugs.

::: src/st/st-widget.c
@@ +953,3 @@
+                                     shadow->blur);
+
+          if (priv->background_image_shadow)

So, I'd omit this check
Comment 21 Florian Müllner 2010-01-04 21:12:56 UTC
Created attachment 150798 [details] [review]
Implement -st-shadow for StWidget

* change return value of st_shadow_texture_create_shadow()
   from gboolean to void

 * do not return NULL from st_shadow_texture_new()

 * change cast and add comment according to owen's review
Comment 22 Owen Taylor 2010-01-05 20:29:41 UTC
Review of attachment 150798 [details] [review]:

Looks great!
Comment 23 Owen Taylor 2010-01-05 20:34:23 UTC
Review of attachment 150184 [details] [review]:

Looks good
Comment 24 Florian Müllner 2010-01-05 20:43:14 UTC
Attachment 150184 [details] pushed as af3b965 - [Overview] Add drop shadows to close buttons
Attachment 150798 [details] pushed as 2dfe113 - Implement -st-shadow for StWidget