GNOME Bugzilla – Bug 603691
Add drop shadows to close buttons in overview
Last modified: 2010-01-05 20:43:27 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.
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.
Created attachment 149004 [details] [review] [Overview] Add drop shadows to close buttons
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.
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)
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
Created attachment 149925 [details] [review] [Overview] Add drop shadows to close buttons Rename -shell-drop-shadow property to box-shadow
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
Review of attachment 149925 [details] [review]: Marking needs-work to correspond to my comment about the box-shadow name.
(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)
Created attachment 150089 [details] [review] Implement -st-shadow for StWidget Updated patch according to Owen's review.
Created attachment 150090 [details] [review] [Overview] Add drop shadows to close buttons Use -st-shadow instead of box-shadow.
Created attachment 150183 [details] [review] Implement -st-shadow for StWidget Update patch to assume blur_radius == blur_window_size instead of blur_radius == sigma.
Created attachment 150184 [details] [review] [Overview] Add drop shadows to close buttons Adjust the shadow values a little
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.)"
(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 ...
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!
(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.)
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
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
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
Review of attachment 150798 [details] [review]: Looks great!
Review of attachment 150184 [details] [review]: Looks good
Attachment 150184 [details] pushed as af3b965 - [Overview] Add drop shadows to close buttons Attachment 150798 [details] pushed as 2dfe113 - Implement -st-shadow for StWidget