GNOME Bugzilla – Bug 651194
Should support border-image as a shorthand property
Last modified: 2011-06-10 20:46:55 UTC
border-image should be supported as a shorthand property, with at least border-image-slice, border-image-repeat and border-image-source composing it. We should also be able to set a gradient as source, instead of an url. Attached patchset contains an implementation of this.
Created attachment 188707 [details] [review] border-image: change Gtk9Slice to GtkBorderImage It's a similar private type, which is much more powerful, and respects the CSS rendering specification for border-image.
Created attachment 188708 [details] [review] styleproperty: turn border-image into a shorthand property It's composed by - border-image-source: a cairo_pattern_t holding an image from file or a gradient - border-image-slice: a GtkBorder containing the slice offsets to apply on the image - border-image-repeat: a GtkRepeatType for the image We deviate from pure CSS3 in the following ways: * border-image-width is assumed to be always 1, i.e. always equal to what's specified by border-width. I don't think it's a particularly useful property to have, but we could add it later if needed. * border-image-outset is absent, as we can't render outside of the allocation yet.
Review of attachment 188707 [details] [review]: Apart from these comments, looks reasonable. Need to update the docs to explain the new possibilities, though. ::: gtk/gtkborderimage.c @@ +283,3 @@ + return _gtk_border_image_ref (image); + + if (!gtk_gradient_resolve (image->source_gradient, props, &pattern)) Need to check if source_gradient != NULL ? Or do you have an implicit condition that source_gradient == NULL implies resolved == TRUE ? @@ +399,3 @@ + height / image_height); + cairo_set_source_surface (cr, surface, 0, 0); + cairo_pattern_set_filter (cairo_get_source (cr), CAIRO_FILTER_NEAREST); why using the nearest filter here ? @@ +532,3 @@ +void +_gtk_border_image_render (GtkBorderImage *image, + GtkThemingEngine *engine, Are you sure that its right to pass the engine in here ? Maybe just pass the border ? ::: gtk/gtkthemingengine.c @@ +2097,1 @@ "border-style", &border_style, Here would be a natural place to get the border to pass into the render function
Created attachment 188787 [details] [review] border-image: change Gtk9Slice to GtkBorderImage It's a similar private type, which is much more powerful, and respects the CSS rendering specification for border-image.
Created attachment 188788 [details] [review] styleproperty: turn border-image into a shorthand property It's composed by - border-image-source: a cairo_pattern_t holding an image from file or a gradient - border-image-slice: a GtkBorder containing the slice offsets to apply on the image - border-image-repeat: a GtkRepeatType for the image We deviate from pure CSS3 in the following ways: * border-image-width is assumed to be always 1, i.e. always equal to what's specified by border-width. I don't think it's a particularly useful property to have, but we could add it later if needed. * border-image-outset is absent, as we can't render outside of the allocation yet.
Created attachment 188789 [details] [review] reftests: add reftests for border-image with gradient and url sources
Created attachment 188790 [details] [review] border-image: ensure image->resolved is set when resolving the gradient
(In reply to comment #3) > Apart from these comments, looks reasonable. > Need to update the docs to explain the new possibilities, though. Thanks for the review, I'll have a go at updating the docs this weekend. This new patchset also adds some reftests and implementations for the ROUND and SCALE repeat styles. > Need to check if source_gradient != NULL ? > > Or do you have an implicit condition that source_gradient == NULL implies > resolved == TRUE ? Yes, if source_gradient is NULL the pattern is there, so the shadow is resolved. I fixed some related confusion wrt. resolution with the last patch attached. > why using the nearest filter here ? I added a comment in the code. We need the nearest filter or otherwise cairo will try to blend in adjacent colors when upscaling. > Are you sure that its right to pass the engine in here ? > Maybe just pass the border ? > Here would be a natural place to get the border to pass into the render > function Yeah, makes sense, fixed now.
Created attachment 188936 [details] [review] docs: expand the docs for border-image
Looks good to me.
There's a few things I'm not sure about here: I think the current CSS specs define the object size of images with no intrinsic dimensions for border-image as the border box. Or said differently: A gradient will always be scaled to the size of the border, which effectively makes the 9slice not do anything but define how much of the image will be cut off. And the image repeat is useless, too, because the gradient fits exactly anyway. We do a bunch of caching inside GtkBorderImage here, but it's now a shorthand property. And shorthands are created on-demand, so no caching happens. So all the caching will be removed immediately after we're done rendering. So I guess we can simplify the code and just remove the caching. If it turns out to be a performance problem, we can work on it then. You're doing resolving of border images, but shorthands are never resolved. They are always assumed to be resolved when they get packed. Shorthands (currently ;)) work like this: 1) They're defined like "shorthand: foo bar 1 2 3;" in the CSS file 2) The CSS parser parses them like any other property and adds them to the ruleset with gtk_css_ruleset_add(). 3) gtk_css_ruleset_add() unpacks the property into the real properties and adds those to the hashtable. So a GtkCssRuleset _never_ contains a shorthand property. 4) Because of (3), when creating the GtkStyleProperties from the css provider, no shorthands are added. 4b) If shorthands are added by other style providers (GtkSettings adds a "font" currently, and I have made that a shorthand in one of my branches), it gets unpacked in _gtk_style_properties_set_property_by_property(). 5) Because of (4) GtkStyleProperties never contain any shorthands. 6) If a shorthand is requested, gtk_style_properties_get_property() and gtk_style_properties_get_valist() pack the property when it is requested. 7) Because packing uses gtk_style_properties_get(), it will only pack from properties that already are resolved. So shorthands are by design never stored anywhere, they are really treated as "shorthands": They give you quick access to stuff without having to write the long version. They're also never faster than using the real properties.
This is really good information - would be great to have it in a code comment somewhere...
This is now fixed in master.