GNOME Bugzilla – Bug 636976
Allow background images to be styled with css
Last modified: 2011-01-24 17:25:57 UTC
When showing face images, it's useful to have a round border aroudn the image. Obviously this is a subjective thing, and should be left to the designers to figure out. ideally, we'd be able to set up the border through CSS. It doesn't work right now, because we draw the background over the border.
Created attachment 176190 [details] [review] StThemeNodeDrawing: clip background to border This allows creating "rounded frames" around images from the CSS.
I'm not sure about this border overlap stuff. it's a little hacky to overlap at all. But even so, there may be a better way of figuring out how much to overlap than the radius / 100.0 bit i tossed in after some trial and error.
Comment on attachment 176190 [details] [review] StThemeNodeDrawing: clip background to border I talked to Owen about this on IRC. He's not happy with the idea of using clipping at all, since it introduces aliasing where the border curves.
Created attachment 176335 [details] [review] StThemeNode: rename border_shadow_material to shadow_material It's a shadow for the whole node, not just for the border.
Created attachment 176336 [details] [review] StThemeNode: split border_texture into two vars The border_texture (and border_material) variable is being overloaded for two purposes: it's used as a source to 9-slice the border from, and it's used as place to prerender the background and border together for gradients. While we only do one or the other for any given node, the two cases are distinct, and should use distinct variables for readability.
Created attachment 176337 [details] [review] StThemeNodeDrawing: generalize render_gradient to render_background_with_border A lot of the border drawing logic in st_theme_node_render_gradient is applicable to other non-solid background types than gradients. This commit refactors that code so that support for other non-solid background types can be more easily integrated later.
Created attachment 176338 [details] [review] StTextureCache: add api to load image to cairo surface Loading a pixbuf in a way that cairo can use it is a pretty involved process that involves a lot of code, and pixel fiddling. This commit adds the mechanism to StTextureCache so we can reuse the existing pixbuf handling code there, and also get caching.
Created attachment 176339 [details] [review] StThemeDrawing: support borders and backgrounds together Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
The above patchset is a first stab at making backgrounds with borders use cairo for nicer rendering. Some open questions: - Right now (even before this patchset) gradients go up to the inside edges of borders instead of the outside edges of borders. I think that's a more convenient and useful mode of operation, but I'm not sure it matches the CSS spec (without changing background-clip). But it might, I need to look and experiment in a browser I think. - With the patchset, we still don't support background images with translucency on top of gradients. Maybe we should? - The current set of test cases don't excerise this new code very well. I should probably augment one of the existing tests or add a new one to make sure things are working how I think they're working.
Created attachment 177574 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images.
Created attachment 177575 [details] [review] StThemeNode: rename border_shadow_material to shadow_material It's a shadow for the whole node, not just for the border.
Created attachment 177576 [details] [review] StThemeNode: split border_texture into two vars The border_texture (and border_material) variable is being overloaded for two purposes: it's used as a source to 9-slice the border from, and it's used as place to prerender the background and border together for gradients. While we only do one or the other for any given node, the two cases are distinct, and should use distinct variables for readability.
Created attachment 177577 [details] [review] StThemeNodeDrawing: generalize render_gradient to render_background_with_border A lot of the border drawing logic in st_theme_node_render_gradient is applicable to other non-solid background types than gradients. This commit refactors that code so that support for other non-solid background types can be more easily integrated later.
Created attachment 177578 [details] [review] StTextureCache: add api to load image to cairo surface Loading a pixbuf in a way that cairo can use it is a pretty involved process that involves a lot of code, and pixel fiddling. This commit adds the mechanism to StTextureCache so we can reuse the existing pixbuf handling code there, and also get caching.
Created attachment 177579 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
So i've started looking back into this post-holidays and I've updated the patchset to work with the changes that have gone into master since comment 9. To respond to some of the previously mentioned open questions: - I looked in the CSS spec and couldn't find any reference to support for background gradients. Firefox seems to have support using -moz-linear-gradient and friends, though. As far as I can tell, it ignores background-clip entirely for gradients in my brief tests and always draws gradients within the bounds of the inside edges of the borders. That's different than how background images work. attachment 177579 [details] [review] makes backgrounds and gradients behave the same way (so they look as if they start behind the border), but we may want to flip gradients back to the other behavior. - I still don't support background images on top of background gradients. Not sure it matters that much. - I've augmented the borders.js test case to show background gradients and background images with curved borders.
Created attachment 177589 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate. This fixes a leak I just noticed when rereading through the patch.
Review of attachment 177575 [details] [review]: Generally looks good, but - the change in st-theme-node-private.h should be moved into this commit - shadow_material is a very generic name (given that there is another shadow material); maybe background_shadow_material or node_shadow_material instead of border_shadow_material and background_image_shadow_material for background_shadow_material?
Review of attachment 177576 [details] [review]: Looks good. ::: src/st/st-theme-node-private.h @@ +97,2 @@ CoglHandle background_shadow_material; + CoglHandle shadow_material; As noted before, this belongs in the previous patch
Review of attachment 177577 [details] [review]: Looking good - some minor comments below. ::: src/st/st-theme-node-drawing.c @@ +429,3 @@ +static cairo_pattern_t * +get_cairo_pattern_of_background_gradient (StThemeNode *node) Just a personal preference, but I prefer create_foo() for functions which allocate memory and are not object methods. @@ +432,3 @@ +{ + cairo_pattern_t *pattern; + This is an internal function, but it may still be useful to add a g_return_val_if_fail (node->background_gradient_type != ST_GRADIENT_NONE, NULL); @@ +464,3 @@ + * @node: #StThemeNode to render + * + * Private function. Of course, it's static - I'd make this a normal comment and remove the 'private' annotation.
Review of attachment 177578 [details] [review]: Looks good. ::: src/st/st-texture-cache.c @@ +752,3 @@ + cairo_surface_reference (surface); + cairo_destroy (cr); + cairo_surface_destroy (dummy_surface); Out of curiosity - is there any advantage in using a dummy surface over surface = cairo_image_surface_create (gdk_pixbuf_get_has_alpha (pixbuf) ? CAIRO_FORMAT_ARGB32 : CAIRO_FORMAT_RGB24, gdk_pixbuf_get_width (pixbuf), gdk_pixbuf_get_height (pixbuf)); cr = cairo_create (surface); gdk_cairo_set_source_pixbuf (cr, pixbuf, 0, 0); cairo_paint (cr); cairo_destroy (cr);
Review of attachment 177589 [details] [review]: Looks mostly good, but I'm not really convinced by the simple rule for clipping (e.g. always use the clipping code path if the node has a border) - especially the rule "background images can be given a shadow using -st-shadow if there is no border" doesn't sound very intuitive (and neither does "background images are clipped to rounded corners only if there is also a border") ::: src/st/st-theme-node-drawing.c @@ +461,3 @@ +static cairo_pattern_t * +get_cairo_pattern_of_background_image (StThemeNode *node, + gboolean *needs_background_fill) As mentioned in a previous review, my personal preference would be create_foo() in this case (feel free to ignore though) @@ +492,3 @@ + + height_ratio = file_height / (node->alloc_height); + width_ratio = file_width / (node->alloc_width); Unnecessary parenthesis @@ +597,3 @@ + /* TODO - support translucent background images on top of gradients + * instead of either/or ? Hmmm, that's a regression, right? OK with me, given that (1) it's not used anywhere (2) at least mozilla's take on CSS gradients[0] are functions used with the background-image property [0] https://developer.mozilla.org/en/CSS/-moz-linear-gradient @@ +612,3 @@ + if (background_image != NULL) + pattern = get_cairo_pattern_of_background_image (node, + &draw_solid_background); If background_image == NULL, draw_solid_background is used uninitialized later (gcc rightfully complains on F14) @@ +881,3 @@ } + else if (background_image && has_border) + { Hmmm - not entirely sure about that: (1) if the background image doesn't need clipping, we could still use the optimized path to draw the background (2) it is not really obvious why background images are not clipped when there is no border - the standard[0] certainly suggests otherwise (obviously we deviate from the standard when scaling down background images, but that's not necessarily an argument for not clipping to rounded corners) Another issue: When the theme node has an -st-shadow property, background images have a shadow if the node has no border, but not if there is one. Prerendering the shadow with the image is probably a bit tough, so documenting that background images don't have shadows if the image is clipped sounds OK to me, if there's a better rule for clipping (e.g. image doesn't fit unscaled into the rectangle marked by borders/border-radii) [0] http://www.w3.org/TR/css3-background/#corner-clipping @@ +1415,3 @@ + * - We clip the background image to the inside edges of the border + * instead of the outside edges of the border (but position the image + * such that it's aligned to the outside edges) The old comment is still true for rounded backgrounds without border.
Review of attachment 177574 [details] [review]: Looks good - there's quite a bit of code repetition in the new test cases, so maybe a helper function would make sense. ::: tests/interactive/borders.js @@ +74,3 @@ +box.add(new St.Label({ text: "Rounded, framed gradients" })); + +let framedGradients = new St.BoxLayout({ vertical: false, Could be omitted, as BoxLayout defaults to horizontal anyway @@ +130,3 @@ +box.add(new St.Label({ text: "Rounded, framed images" })); + +let framedImages = new St.BoxLayout({ vertical: false, Could be omitted here as well
Created attachment 178175 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images.
Comment on attachment 177575 [details] [review] StThemeNode: rename border_shadow_material to shadow_material (In reply to comment #18) > Review of attachment 177575 [details] [review]: > > Generally looks good, but > - the change in st-theme-node-private.h should be moved > into this commit > - shadow_material is a very generic name (given that > there is another shadow material); maybe background_shadow_material > or node_shadow_material instead of border_shadow_material and > background_image_shadow_material for background_shadow_material? I think I'll just drop this patch. It doesn't help my ultimate goal here and it's even debatable that the old name is bad. CSS for instance calls the area the "border-box" so border_shadow_material actually isn't that bad.
(In reply to comment #21) > Out of curiosity - is there any advantage in using a dummy surface over > > surface = cairo_image_surface_create (gdk_pixbuf_get_has_alpha (pixbuf) ? > CAIRO_FORMAT_ARGB32 > : > CAIRO_FORMAT_RGB24, > gdk_pixbuf_get_width (pixbuf), > gdk_pixbuf_get_height (pixbuf)); > cr = cairo_create (surface); > gdk_cairo_set_source_pixbuf (cr, pixbuf, 0, 0); > cairo_paint (cr); > cairo_destroy (cr); I don't think there's a big advantage, just slightly more optimized to do it my way.
(In reply to comment #26) > I don't think there's a big advantage, just slightly more optimized to do it my > way. Sounds like a good enough reason to me - I was just unsure whether the creation/destruction overhead was greater than the cost of paint or not.
(In reply to comment #22) > Review of attachment 177589 [details] [review]: > > Looks mostly good, but I'm not really convinced by the simple rule for clipping > (e.g. always use the clipping code path if the node has a border) - especially > the rule "background images can be given a shadow using -st-shadow if there is > no border" doesn't sound very intuitive (and neither does "background images > are clipped to rounded corners only if there is also a border") I think maybe we should have two different properties -st-shadow and -st-background-shadow or some such. Trying to come up for heuristics when to apply the shadow to the border box and when to apply the shadow to the background image seems fragile to me. Thoughts? > @@ +612,3 @@ > + if (background_image != NULL) > + pattern = get_cairo_pattern_of_background_image (node, > + > &draw_solid_background); > > If background_image == NULL, draw_solid_background is used uninitialized later > (gcc rightfully complains on F14) Ah, thanks. I don't get the warning here (probably because I compile with -O0 by default I guess) > > @@ +881,3 @@ > } > + else if (background_image && has_border) > + { > > Hmmm - not entirely sure about that: > > (1) if the background image doesn't need clipping, we could > still use the optimized path to draw the background True, we could optimize this case a bit. > > (2) it is not really obvious why background images are not > clipped when there is no border - the standard[0] certainly > suggests otherwise > (obviously we deviate from the standard when scaling down > background images, but that's not necessarily an argument > for not clipping to rounded corners) Right, I guess we need to go through the slow case anytime there's a border radius, and we should clip in the fast case as well. > Another issue: When the theme node has an -st-shadow property, background > images have a shadow if the node has no border, but not if there is one. > Prerendering the shadow with the image is probably a bit tough, so documenting > that background images don't have shadows if the image is clipped sounds OK I doesn't really sit well with me that the generic fallback case doesn't handle some things the fast path handles. I've looked into it a bit, and it's not that bad to get shadows in the prerendered case, so I think we should go that route.
(In reply to comment #28) > I think maybe we should have two different properties -st-shadow and > -st-background-shadow or some such. Trying to come up for heuristics when to > apply the shadow to the border box and when to apply the shadow to the > background image seems fragile to me. > > Thoughts? Currently -st-shadow applies to both (though currently it's not used, e.g. there's either a background or a background-image). I wouldn't mind splitting it - in that case, we could call the one which applies to the background "box-shadow" ;-) (I think so - that's what the CSS3 property does, right? Maybe -st-background-image-shadow for the other property ...) > Right, I guess we need to go through the slow case anytime there's a border > radius, and we should clip in the fast case as well. You mean border-radius + border-image, right? I'm not sure what you mean by clipping in the fast path - if the background image is clipped (and I can't think of anything other that requires clipping), we always choose the slow path, don't we? > I doesn't really sit well with me that the generic fallback case doesn't handle > some things the fast path handles. I've looked into it a bit, and it's not > that bad to get shadows in the prerendered case, so I think we should go that > route. I was assuming that shadows would hardly be visible when an image is clipped, but if you think that you can handle shadows there, that's better :-)
(In reply to comment #29) > (In reply to comment #28) > > I think maybe we should have two different properties -st-shadow and > > -st-background-shadow or some such. Trying to come up for heuristics when to > > apply the shadow to the border box and when to apply the shadow to the > > background image seems fragile to me. > > > > Thoughts? > > I wouldn't mind splitting it - in that case, we could call the one which > applies to the background "box-shadow" ;-) > (I think so - that's what the CSS3 property does, right? Maybe > -st-background-image-shadow for the other property ...) -st-background-image-shadow sounds fine to me. It might not be worth the code churn to rename -st-shadow to -st-box-shadow though. > > Right, I guess we need to go through the slow case anytime there's a border > > radius, and we should clip in the fast case as well. > > You mean border-radius + border-image, right? I haven't really thought about how border-images fit in. I guess they should ultimately end up supported by the cairo code as well. I don't want to that right now, though. We're already creeping the scope a bit here. I just meant if we've got round corners but no border, we still need need to use cairo to get nice round corners. > I'm not sure what you mean by > clipping in the fast path - if the background image is clipped (and I can't > think of anything other that requires clipping), we always choose the slow > path, don't we? We still choose the fast path if the node isn't rounded and it doesn't have borders. If you have a background-position set (so we don't scale) then we can draw outside of the allocation. I thought that's what you meant above by, "background images are not clipped when there is no border"
(In reply to comment #30) > -st-background-image-shadow sounds fine to me. It might not be worth the code > churn to rename -st-shadow to -st-box-shadow though. I meant box-shadow (without prefix), as in the current draft[0]. We use a simplified syntax though ... [0] http://www.w3.org/TR/css3-background/#box-shadow > > > Right, I guess we need to go through the slow case anytime there's a border > > > radius, and we should clip in the fast case as well. > > > > You mean border-radius + border-image, right? > > I haven't really thought about how border-images fit in. I guess they should > ultimately end up supported by the cairo code as well. I don't want to that > right now, though. We're already creeping the scope a bit here. Sorry, I meant border-radius + background-image - I don't see any need for clipping when no background image is used, as the background itself does not require clipping anyway. (Speaking about border-image though - if I recall correctly, it is supposed to be always unclipped. But yeah, let's ignore it and move on). > I just meant if we've got round corners but no border, we still need to > use cairo to get nice round corners. Ah, OK - yeah, cairo is used to draw the (cached) corner textures for the 9-slice. > > I'm not sure what you mean by > > clipping in the fast path - if the background image is clipped (and I can't > > think of anything other that requires clipping), we always choose the slow > > path, don't we? > We still choose the fast path if the node isn't rounded and it doesn't have > borders. > > If you have a background-position set (so we don't scale) then we can draw > outside of the allocation. I thought that's what you meant above by, > "background images are not clipped when there is no border" Ah, no - I was referring to the case where we have rounded corners and no border, e.g. clip the background image to the corners. I didn't think of drawing outside the allocation when background-position is set - I guess that case can be handled easily with cogl_clip_push_rectangle() though?
Created attachment 178281 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images.
Created attachment 178282 [details] [review] StThemeNode: Split -st-shadow into two attributes Currently, "-st-shadow" can mean one of two very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node When it means one or the other isn't that well defined, so this commit adds a new "-st-background-image-shadow" property to make it all very explicit.
Created attachment 178283 [details] [review] StThemeNode: split border_texture into two vars The border_texture (and border_material) variable is being overloaded for two purposes: it's used as a source to 9-slice the border from, and it's used as place to prerender the background and border together for gradients. While we only do one or the other for any given node, the two cases are distinct, and should use distinct variables for readability.
Created attachment 178284 [details] [review] StThemeNodeDrawing: clip background image to bounds When drawing the background image, we need to make sure we don't draw outside the boundry of the node.
Created attachment 178285 [details] [review] StThemeNodeDrawing: generalize render_gradient to render_background_with_border A lot of the border drawing logic in st_theme_node_render_gradient is applicable to other non-solid background types than gradients. This commit refactors that code so that support for other non-solid background types can be more easily integrated later.
Created attachment 178286 [details] [review] StTextureCache: add api to load image to cairo surface Loading a pixbuf in a way that cairo can use it is a pretty involved process that involves a lot of code, and pixel fiddling. This commit adds the mechanism to StTextureCache so we can reuse the existing pixbuf handling code there, and also get caching.
Created attachment 178287 [details] [review] st-private: split pixel blurring code out The guts are somewhat complicated, and are potentially reusable for future cairo fallback code.
Created attachment 178288 [details] [review] st-private: add cairo code for drawing shadow This does the same thing as the cogl equivalent code, but for handling fallbacks.
Created attachment 178289 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
Review of attachment 178281 [details] [review]: Looks good. ::: tests/interactive/borders.js @@ +85,3 @@ + + '-st-shadow: -2px 2px 6px rgba(0,0,0,0.5); ' + + 'width: 32px;' + + 'height: 32px;' }); The last three properties are static and could be moved into test.css ... @@ +108,3 @@ + style: 'border: ' + borderWidth + 'px solid #8b8b8b;' + + 'border-radius: ' + borderRadius + 'px;' + + '-st-shadow: -2px 2px 6px rgba(0,0,0,0.5); ' ... and this one as well.
Review of attachment 178282 [details] [review]: Code looks good, just one objection: ::: src/st/st-icon.c @@ +251,3 @@ { StThemeNode *node = st_widget_get_theme_node (ST_WIDGET (actor)); + StShadow *shadow_spec = st_theme_node_get_background_image_shadow (node); Yeah, I forgot about StIcon yesterday, but I see you found it anyway :-) I think it should get its own property though - the icon is actually content, so it's perfectly valid to specify a separate background image beneath it. My suggestion would be "icon-shadow" (there's already "icon-size").
Review of attachment 178283 [details] [review]: Looks good. ::: src/st/st-theme-node-drawing.c @@ +1264,3 @@ paint_opacity); + /* Gradients and border images are mutually exclusive at this time */ I didn't notice that in the last review, but is there still a valid reason for that limitation? I'd imagine that if (node->prerendered_material != COGL_INVALID_HANDLE) paint_material_with_opacity (...); if (node->border_slices_material != COGL_INVALID_HANDLE) st_theme_node_paint_sliced_border_image (...); should work now, right? If I'm wrong ignore the comment, the limitation has been there for ages and no one complained so far ...
Review of attachment 178284 [details] [review]: Hmmm, does "bounds" equal "bounding rectangle"? From the commit message I'd assume that rounded corners are taken into account, so it may be a good idea to clear the message up a bit. But then I'm not a native speaker, so I could very well be wrong. ::: src/st/st-theme-node-drawing.c @@ +1292,3 @@ * like boder and background color into account). */ + cogl_clip_push_rectangle (allocation.x1, allocation.y1, allocation.x2, allocation.y2); Can you move that above the comment? It's a bit weird to have a largish comment talking about shadows, followed by an operation which is apparently unrelated ;-)
Review of attachment 178285 [details] [review]: Looks good.
Review of attachment 178286 [details] [review]: I assume the patch is reattached unchanged - please complain if not :-)
Hi, > ::: tests/interactive/borders.js > @@ +85,3 @@ > + + '-st-shadow: -2px 2px 6px > rgba(0,0,0,0.5); ' > + + 'width: 32px;' > + + 'height: 32px;' }); > > The last three properties are static and could be moved into test.css ... Well that style class is used by other test cases. I don't want to mess with it. > @@ +108,3 @@ > + style: 'border: ' + borderWidth + 'px > solid #8b8b8b;' > + + 'border-radius: ' + > borderRadius + 'px;' > + + '-st-shadow: -2px 2px 6px > rgba(0,0,0,0.5); ' I think it would be a little weird to have class named .background-image which has enforced shadows. We could rename it .background-image-with-shadows or something but then it's less generally useful. I don't think there's a real advantage to moving all the static content to the css file either, honestly.
Review of attachment 178287 [details] [review]: Looks good.
(In reply to comment #47) > Well that style class is used by other test cases. OK, I wasn't aware of that - makes sense to have this locally then.
Review of attachment 178289 [details] [review]: Woot, shadows! Still some comments left, but nothing tricky ::: src/st/st-theme-node-drawing.c @@ +570,1 @@ gboolean draw_solid_background; Ooops! ... you did it again. You messed with my build. Seriously, gcc (4.5.1) still complains about draw_solid_background being possibly used uninitialized - no idea why though, maybe it doesn't like passing the address of an uninitialized variable (to create_cairo_pattern_of_background_image()) @@ +762,3 @@ + cairo_pattern_t *shadow_pattern; + + g_assert (shadow_spec != NULL); Doesn't hurt(*), but seems unnecessary: draw_background_shadow = FALSE; if (shadow_spec) draw_background_shadow = TRUE; if (draw_background_shadow) assert (shadow_spec != NULL); (*) as in: I don't mind at all leaving it as-is @@ +763,3 @@ + + g_assert (shadow_spec != NULL); + g_assert (pattern != NULL); Uhm - should an error when loading an image file (e.g. a typo in the CSS) really "crash" the window manager? @@ +932,3 @@ node->prerendered_texture = st_theme_node_render_background_with_border (node); } + else if (background_image && (has_border || has_border_radius)) Hmmm - in a case like node { height: 50px; width: 50px; border: 2px solid black; border-radius: 5px; background-image: url("foo.png"); } where foo.png is 32x32, clipping would not be required and the fast path could be used. I think for now it's OK to just add a comment about possible future optimizations here.
Review of attachment 178288 [details] [review]: Found the code very hard to figure out, but basically looks right to me. You may be able to extract some explanatory commentary from my comments below. One suggest about the way you copy to a temporary A8 surface. ::: src/st/st-private.c @@ +658,3 @@ + cairo_pattern_get_surface (src_pattern, &src_surface); + + g_return_val_if_fail (cairo_surface_get_type (src_surface) == CAIRO_SURFACE_TYPE_IMAGE, NULL); But what if the pattern wasn't a surface pattern at all? If you are trying to be careful, you should check the return value from get_surface to be CAIRO_PATTERN_TYPE_SURFACE, or just take a cairo_surface_t as the parameter @@ +665,3 @@ + surface_in = cairo_surface_create_similar (src_surface, + CAIRO_CONTENT_ALPHA, + width_in, height_in); I don't think you want create_similar here - you want to create a CAIRO_FORMAT_A8 image surface here always, sinc ethat what you need below. So you could remove the check on get_type() above. (Though if you want to get fancy, maybe you should check if the surface is already an A8 image surface and skip the copy, and just ref the source surface) Also need a comment explaining why you are creating this temporary. @@ +689,3 @@ + cairo_surface_destroy (surface_out); + + cairo_pattern_get_matrix (src_pattern, &shadow_matrix); Gosh, I detest thinking about pattern matrices and matrix transformations. For my reference: * The pattern matrix is the transformation from user space to pattern space * cairo_matrix_<x> prepends the scale/etc, so the effect is that we first do the new thing, then the original matrix. @@ +694,3 @@ + cairo_matrix_translate (&shadow_matrix, + (width_out - width_in) / 2.0, + (height_out - height_in) / 2.0); So, here to get the source pixel, we first translate by the difference in size over two, then apply the original matrix. OK, so that works only when the original matrix is a pure translate, since otherwise we needed to *append* the translate not prepend the translate - the amount you are translating by here is a distance in pattenr space, not user space. @@ +699,3 @@ + * can scale it without side effects */ + origin_xoffset = shadow_matrix.x0 - width_out / 2.0; + origin_yoffset = shadow_matrix.y0 - height_out / 2.0; Again this seems to be assuming pure translate. Probably should note in a comment that shaodw_matrix.x0/y0 is the point in pattern space that corresponds to the origin of user space - so negative number for an image at a positive x/y. @@ +700,3 @@ + origin_xoffset = shadow_matrix.x0 - width_out / 2.0; + origin_yoffset = shadow_matrix.y0 - height_out / 2.0; + cairo_matrix_translate (&shadow_matrix, -origin_xoffset, -origin_yoffset); unary minuses get a trailing space too. So, this is really the "move it back" if we are thinking about the pattern matrix - because cairo_matrix_translate() is, as above, prepending, so the order of transformations is the opposite order from the order they appear in code. @@ +708,3 @@ + + /* Move it back */ + cairo_matrix_translate (&shadow_matrix, origin_xoffset, origin_yoffset); And this would be the operation that centers around the origin. @@ +713,3 @@ + cairo_matrix_translate (&shadow_matrix, + -shadow_spec->xoffset, + -shadow_spec->yoffset); Hmm, I think the above is OK, assuming pure translate for the original matrix. It probably needs a comment explaining how to think about the operations, something like: /* the pattern matrix is the matrix that transforms from user space to pattern * space. An operation like cairo_matrix_translate() prepends the transformation * to the pattern (does it before the operation of the pattern), but the code here * is written thinking about it in an alternately valid way - of appending the * inverse of the transformation to the inverse of the matrix ... which here * would be the transformation from pattern space to user space. */ But if I wanted to fix the code up to handle arbitrary pattern matrices in the input (which seems sort of pointless for our use case) I'd probably actually rewrite it to invert the matrix, work with the inverse (the matrix transforming from user to pattern), and then invert it. That wouldn't really help with the need to do things in a "backwards" order - it just helps because the stuff we need to do is actually in pattern space as noted above.
Created attachment 178358 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images.
Created attachment 178359 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to hard coded widget in the source When it means any of these isn't that well defined, so this commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit.
Created attachment 178360 [details] [review] StThemeNode: split border_texture into two vars The border_texture (and border_material) variable is being overloaded for two purposes: it's used as a source to 9-slice the border from, and it's used as place to prerender the background and border together for gradients. While we only do one or the other for any given node, the two cases are distinct, and should use distinct variables for readability.
Created attachment 178361 [details] [review] StThemeNode: Don't make border images and gradients mutually exclusive Since we no longer use the same material for both purposes, we can now remove the restriction that they are mutually exclusive.
Created attachment 178363 [details] [review] StThemeNodeDrawing: clip background image to node allocation When drawing the background image, we need to make sure we don't draw outside the bounding rectangle of the node.
Created attachment 178364 [details] [review] st-private: add cairo code for drawing shadow This does the same thing as the cogl equivalent code, but for handling fallbacks.
Created attachment 178365 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
Created attachment 178366 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to hard coded widget in the source It isn't well defined which of the above 3 cases -st-shadow will mean for any given node, however. This commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit. This attachment is a minor improvement over attachment 178359 [details] [review] because I decided to do a make clean; make CFLAGS="$CFLAGS -O0" and it found another case of uninitialized variables.
(i meant CFLAGS="$CFLAGS -O2" above, not O0)
Review of attachment 178358 [details] [review]: Sorry that I missed that earlier, but you need to add testcommon/face-plain.png to the TEST_JS variable.
Created attachment 178548 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images. This is like attachment 178358 [details] [review] but updates the Makefile as well.
Created attachment 178655 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with gradients and background images.
Created attachment 178656 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to hard coded widget in the source It isn't well defined which of the above 3 cases -st-shadow will mean for any given node, however. This commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit.
Created attachment 178657 [details] [review] StThemeNode: split border_texture into two vars The border_texture (and border_material) variable is being overloaded for two purposes: it's used as a source to 9-slice the border from, and it's used as place to prerender the background and border together for gradients. While we only do one or the other for any given node, the two cases are distinct, and should use distinct variables for readability.
Created attachment 178658 [details] [review] StThemeNode: Don't make border images and gradients mutually exclusive Since we no longer use the same material for both purposes, we can now remove the restriction that they are mutually exclusive.
Created attachment 178659 [details] [review] StThemeNodeDrawing: clip background image to node allocation When drawing the background image, we need to make sure we don't draw outside the bounding rectangle of the node.
Created attachment 178660 [details] [review] StThemeNodeDrawing: generalize render_gradient to render_background_with_border A lot of the border drawing logic in st_theme_node_render_gradient is applicable to other non-solid background types than gradients. This commit refactors that code so that support for other non-solid background types can be more easily integrated later.
Created attachment 178661 [details] [review] StTextureCache: add api to load image to cairo surface Loading a pixbuf in a way that cairo can use it is a pretty involved process that involves a lot of code, and pixel fiddling. This commit adds the mechanism to StTextureCache so we can reuse the existing pixbuf handling code there, and also get caching.
Created attachment 178662 [details] [review] st-private: split pixel blurring code out The guts are somewhat complicated, and are potentially reusable for future cairo fallback code.
Created attachment 178664 [details] [review] st-private: add cairo code for drawing shadow This does the same thing as the cogl equivalent code, but for handling fallbacks.
Created attachment 178665 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
No changes in the latest round, just reattached in the right order (by request of Florian who's helpfully reviewing it for me)
Review of attachment 178655 [details] [review]: OK.
Review of attachment 178659 [details] [review]: The code obviously works as advertised, but unfortunately it's a change in behavior; if hovering over a window preview in the overview, a small close button appears in the top right - the button has a shadow (so far the only use case of -st-background-image-shadow), which is now clipped (and looks rather crappy). It is possible to work around the issue in the CSS, but it's rather ugly: .window-close { background-image: url("close-window.svg"); - height: 24px; - width: 24px; + background-position: 6px 6px; + height: 36px; + width: 36px; -st-background-image-shadow: -2px 2px 6px rgba(0,0,0,0.5); - -shell-close-overlap: 16px; + -shell-close-overlap: 22px; } (quick hack, could likely be improved) Unfortunately I don't see much of an alterative (except for adding a custom property to enable/disable clipping, but that seems overkill)
Review of attachment 178665 [details] [review]: From a quick look the code looks good - unfortunately I found a crasher: ::: src/st/st-theme-node-drawing.c @@ +991,3 @@ + /* Should have been covered above */ + g_assert (!has_border_radius); Triggered by chat notifications' entry, e.g. shell goes boom when expanding the notification.
Review of attachment 178657 [details] [review]: Assuming the patch is unchanged since the last review.
Review of attachment 178658 [details] [review]: Looks mostly good. Apart from the comment below, a test case would be fab ;-) ::: src/st/st-theme-node-drawing.c @@ +1273,1 @@ paint_material_with_opacity (node->prerendered_material, &allocation, paint_opacity); In that order the background is drawn above the border, isn't it?
Review of attachment 178660 [details] [review]: The patch is unchanged, right?
Review of attachment 178664 [details] [review]: Much more readable.
Review of attachment 178656 [details] [review]: Admittedly parse_shadow_property() is extremely sloppy right now (e.g. it doesn't even require x/y offsets), but I really dislike the new parsing code, specifically the way inheritance is handled. I guess a simpler approach would be to change the return value of parse_shadow_property() to gboolean and check n_offsets for being in the range [1,3] at the end of the function and use that in st_theme_node_lookup_shadow(). Also, in the commit message 3) shadow applied to hard coded widget in the source is a very generic description, and does not really makes it obvious why the corresponding property is called "icon-shadow". I'd rather be specific and say 3) shadow applied to the content of an StIcon ::: src/st/st-icon.c @@ +251,3 @@ { StThemeNode *node = st_widget_get_theme_node (ST_WIDGET (actor)); + StShadow *shadow_spec = st_theme_node_get_background_image_shadow (node); No. @@ +374,3 @@ + gint width, height; + + shadow_spec = st_shadow_new (&color, xoffset, yoffset, blur, spread); Leaking. Anyway, it probably makes sense to cache the shadow_spec for use in st_icon_paint(). ::: src/st/st-theme-node.c @@ +2625,3 @@ + GetFromTermResult *blur_result, + gdouble *spread, + GetFromTermResult *spread_result) I don't really like this - in my opinion, shadows should be treated as single properties and therefore either be inherited as a whole or not at all. @@ +2696,3 @@ + * parent's parent, and so forth. Note that if the property has a + * value of 'inherit' it will be inherited even if %FALSE is passed + * in for @inherit; this only affects the default behavior for inheritance. The comment for @inherit sounds right, but does it match the function? As I understand the spec[0], it is possible to do something like node { foo-shadow: inherit; } but the code looks like it parses something like node { foo-shadow: 0px inherit inherit 5px black; } I also think that node { foo-shadow: 4px 4px; } should mean: a black, unblurred shadow with offsets (4,4) (e.g. use reasonable default values for optional parameters) rather than looking for the omitted parameters in parent nodes (if @inherit is %TRUE). [0] http://www.w3.org/TR/CSS21/cascade.html#value-def-inherit ::: src/st/st-theme-node.h @@ +141,3 @@ + gdouble *yoffset, + gdouble *blur, + gdouble *spread); Any particular reason for not using gboolean st_theme_node_lookup_shadow (StThemeNode *node, const char *property_name, gboolean inherit, StShadow *shadow); which looks quite a bit friendlier? @@ +157,3 @@ + gdouble *yoffset, + gdouble *blur, + gdouble *spread); Same question.
(In reply to comment #76) > ::: src/st/st-theme-node-drawing.c > @@ +991,3 @@ > > + /* Should have been covered above */ > + g_assert (!has_border_radius); > > Triggered by chat notifications' entry, e.g. shell goes boom when expanding the > notification. Ahh, that was a thinko, it just doesn't belong there.
(In reply to comment #78) > Review of attachment 178658 [details] [review]: > > Looks mostly good. Apart from the comment below, a test case would be fab ;-) A test case is a good idea, i'll look into it. > ::: src/st/st-theme-node-drawing.c > @@ +1273,1 @@ > paint_material_with_opacity (node->prerendered_material, &allocation, > paint_opacity); > > In that order the background is drawn above the border, isn't it? Right, we've always drawn the background above the border (which shouldn't be a problem as long as the drawing doesn't overlap I guess...) I'll look into doing the test case and playing with this more, though.
(In reply to comment #81) > Also, in the commit message > > 3) shadow applied to hard coded widget in the source > > is a very generic description, and does not really makes it obvious why the > corresponding property is called "icon-shadow". I'd rather be specific and say > > 3) shadow applied to the content of an StIcon sure. > > ::: src/st/st-icon.c > @@ +251,3 @@ > { > StThemeNode *node = st_widget_get_theme_node (ST_WIDGET (actor)); > + StShadow *shadow_spec = st_theme_node_get_background_image_shadow > (node); > > No. woops! > The comment for @inherit sounds right, but does it match the function? As I > understand the spec[0], it is possible to do something like > > node { > foo-shadow: inherit; > } indeed playing around in a browser, the above works > but the code looks like it parses something like > > node { > foo-shadow: 0px inherit inherit 5px black; > } and this doesn't. Only allowing the less flexible way should certainly be a lot less code too. > Any particular reason for not using > > gboolean st_theme_node_lookup_shadow (StThemeNode *node, > const char *property_name, > gboolean inherit, > StShadow *shadow); > > which looks quite a bit friendlier? no, that's fine.
(In reply to comment #75) > Review of attachment 178659 [details] [review]: > > The code obviously works as advertised, but unfortunately it's a change in > behavior; if hovering over a window preview in the overview, a small close > button appears in the top right - the button has a shadow (so far the only use > case of -st-background-image-shadow), which is now clipped (and looks rather > crappy). So my current thinking is this: - We should always clip the background image to the nodes allocation. This makes sure the background never "leaks" out. - We should never clip a background image shadow to the nodes allocation. This way we never get an unintentionally cut off shadow. - if the node has a border (a non-zero border width or border radius), we should clip the background image and background image shadow to the border. There are two implications from this: 1) we should move the cogl_clip_push_rectangle() call after the paint shadow call (this covers the fast path case) 2) The prerendered image may be bigger than the nodes allocation and will need to get sized and positioned appropriately (this covers the cairo case) 1 is easy and should fix the regression you found I think. 2 is a little harder to implement.
(In reply to comment #85) > - We should always clip the background image to the nodes allocation. This > makes sure the background never "leaks" out. > - We should never clip a background image shadow to the nodes allocation. This > way we never get an unintentionally cut off shadow. This works with what we use right now, as the only background-image which has a shadow is unclipped (or rather, the image fits entirely into the clipped area). But I suppose it will look really weird when the image is actually clipped - imagine a circle image, which is clipped to a slice, but draws a full circle shadow. The correct solution is probably to create the shadow from the clipped image and draw it unclipped (if we clip the image, see another proposal below). > - if the node has a border (a non-zero border width or border radius), we > should clip the background image and background image shadow to the border. I think it will be a lot easier if we include the background in that rule, e.g. only clip the background image and its shadow if the node has a border, a border radius, or a background (gradient or solid). In other words, leave background images unclipped if it's the only background/border property. It doesn't make much less sense to me, and shouldn't be hard to implement (only the clip in the fast path needs to be made conditional). Thoughts?
(In reply to comment #86) > The correct solution is probably to create the shadow from the clipped > image and draw it unclipped (if we clip the image, see another proposal below). Right, that's probably the nicest solution. > I think it will be a lot easier if we include the background in that rule, e.g. > only clip the background image and its shadow if the node has a border, a > border radius, or a background (gradient or solid). In other words, leave > background images unclipped if it's the only background/border property. And this is the easy solution. I think given how drawn out this bug report is getting we should go with the "good enough" second solution for now and look toward doing the right solution at some point in the future. I'll look into it.
(In reply to comment #87) > I think given how drawn out this bug report is getting we should go with the > "good enough" second solution for now and look toward doing the right solution > at some point in the future. Agreed. Don't forget to add to the big, scary CSS comment ;-)
Created attachment 179016 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to the content of a StIcon It isn't well defined which of the above 3 cases -st-shadow will mean for any given node, however. This commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit.
Created attachment 179017 [details] [review] StThemeNode: Don't make border images and gradients mutually exclusive Since we no longer use the same material for both purposes, we can now remove the restriction that they are mutually exclusive.
Created attachment 179018 [details] [review] StThemeNodeDrawing: clip background image to node allocation When drawing the background image, we need to make sure we don't draw outside the bounding rectangle of the node.
Created attachment 179019 [details] [review] StThemeNodeDrawing: clip background image shadow to outline When drawing the background image shadow, we need to clip it to the node's background color, gradient, or borders if present. If the background color is transparent, and there aren't any borders, then we don't clip the shadow since there is nothing to confine it.
Created attachment 179021 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
So I ended up doing both the "get shadow from clipped image" solution and the "only clip shadow if there's a border or other visible boundry to confine it to" solution.
oh forgot to post the updated test case commit which includes gradients + border images.
Created attachment 179022 [details] [review] tests: showcase borders with non-solid backgrounds This commit adds a few more examples to borders.js that render borders with various combinations of gradients, background images, shadows, and border-images.
Review of attachment 179022 [details] [review]: Looks good, though you could avoid horizontal scrolling if you increase the stage width slightly (~20 px).
Review of attachment 179016 [details] [review]: Much less confusing handling 'inherit' that way. Comments left are pretty minor. ::: src/st/st-icon.c @@ +375,3 @@ + if (st_theme_node_lookup_shadow (theme_node, "icon-shadow", FALSE, + &priv->shadow_spec)) + { I think it's better to update shadow_spec in st_icon_style_changed() - the shadow material needs to be updated on allocation and icon-theme changes, the spec doesn't. So adding if (priv->shadow_spec) st_shadow_unref (priv->shadow_spec); priv->shadow_spec = st_theme_node_get_shadow (theme_node, "icon-shadow"); to st_icon_style_changed() should save iterating over the node's properties a couple of times. ::: src/st/st-theme-node.c @@ +2647,3 @@ + { + /* we only allow inherit on the line by itself + */ Nitpick: Single line comments can be on one line (I'm not sure what our "offical" style is here, but quickly skimming through the file suggests that and it looks nicer to me - still, feel free to ignore ...) @@ +2695,3 @@ } + + if (n_offsets >= 2) Could use a short comment about offsets being required and other parameters being optional @@ +2811,3 @@ + + g_warning ("Did not find shadow property '%s'", property_name); + shadow = st_shadow_new (&color, 0., 0., 0., 0.); Not sure - returning a fallback shadow spec is more convenient for JS, but I don't see us blurring pixels in JS any time soon. For C code I think silently returning NULL is more convenient.
Review of attachment 179017 [details] [review]: Looks good.
Review of attachment 179018 [details] [review]: OK.
Review of attachment 179019 [details] [review]: OK.
Review of attachment 179021 [details] [review]: Looks good, some minor comments. ::: src/st/st-theme-node-drawing.c @@ +508,3 @@ + + if (surface == NULL) + return NULL; Oh, that's what gcc was complaining about - if returning early here, needs_background_fill is left unset. @@ +776,3 @@ + } + else + surface_box = node_box; Can you use st_theme_node_get_paint_box (node, &node_box, &surface_box) here? @@ +1722,3 @@ + } + else + box = allocation; Again, does it make sense to use st_theme_node_get_paint_box()?
(In reply to comment #102) > Review of attachment 179021 [details] [review]: > > Looks good, some minor comments. > > ::: src/st/st-theme-node-drawing.c > @@ +508,3 @@ > + > + if (surface == NULL) > + return NULL; > > Oh, that's what gcc was complaining about - if returning early here, > needs_background_fill is left unset. Right. I think it's pretty normal to leave out variables untouched in the case of failure though, so I fixed this earlier by initializing the variable in the caller. > Can you use st_theme_node_get_paint_box (node, &node_box, &surface_box) here? [...] > Again, does it make sense to use st_theme_node_get_paint_box()? Not without fixing get_paint_box() first. That's a fix that should probably happen, though, and seems like a nice clean up.
Created attachment 179183 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to the content of a StIcon It isn't well defined which of the above 3 cases -st-shadow will mean for any given node, however. This commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit.
Created attachment 179184 [details] [review] StThemeDrawing: clip background to border Previously, trying to use a background image and border on the same node resulted in the background drawing over the border. This commit adds support for background images to st_theme_node_render_background_with_border and changes the code to call that function when appropriate.
Review of attachment 179183 [details] [review]: Mostly good. ::: src/st/st-icon.c @@ +290,3 @@ + priv->shadow_spec = NULL; + } + priv->shadow_spec = st_theme_node_get_shadow (theme_node, "icon-shadow"); Ah, sorry. When I suggested that code, I assumed that st_theme_node_get_shadow() would return NULL if the property is not found. As you didn't make that change, you'll need st_theme_node_lookup_shadow() here. ::: src/st/st-theme-node.c @@ +3268,3 @@ } + *paint_box = *actor_box; Could be moved up as *paint_box = *actor_box; if (!box_shadow && !background_image_shadow && !outline_width) return;
Review of attachment 179184 [details] [review]: OK. ::: src/st/st-theme-node-drawing.c @@ +761,3 @@ + paint_box.x1 += - paint_box.x1; + paint_box.y2 += - paint_box.y1; + paint_box.y1 += - paint_box.y1; I think paint_box.x1 = paint_box.y1 = 0 is a bit clearer, but OK. @@ +764,3 @@ + + rowstride = cairo_format_stride_for_width (CAIRO_FORMAT_ARGB32, + paint_box.x2 - paint_box.x1); Of course, with the above translation you could just as well use paint_box.x2 for the width ...
Hi, > I assumed that st_theme_node_get_shadow() would return NULL if the property is not found. Whee. Of course, I meant to make that change, too.
Created attachment 179189 [details] [review] StThemeNode: Split -st-shadow into three attributes Currently, "-st-shadow" can mean one of three very different things: 1) shadow based on alpha of the background image 2) shadow the "border box" of the node 3) shadow applied to the content of a StIcon It isn't well defined which of the above 3 cases -st-shadow will mean for any given node, however. This commit splits the property into three different properties, "box-shadow", "-st-background-image-shadow", and "icon-shadow" to make it all very explicit.
Review of attachment 179189 [details] [review]: Yay!