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 651194 - Should support border-image as a shorthand property
Should support border-image as a shorthand property
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2011-05-26 21:58 UTC by Cosimo Cecchi
Modified: 2011-06-10 20:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
border-image: change Gtk9Slice to GtkBorderImage (41.73 KB, patch)
2011-05-26 21:58 UTC, Cosimo Cecchi
reviewed Details | Review
styleproperty: turn border-image into a shorthand property (10.80 KB, patch)
2011-05-26 21:58 UTC, Cosimo Cecchi
none Details | Review
border-image: change Gtk9Slice to GtkBorderImage (44.52 KB, patch)
2011-05-27 22:07 UTC, Cosimo Cecchi
none Details | Review
styleproperty: turn border-image into a shorthand property (10.80 KB, patch)
2011-05-27 22:07 UTC, Cosimo Cecchi
none Details | Review
reftests: add reftests for border-image with gradient and url sources (6.84 KB, patch)
2011-05-27 22:07 UTC, Cosimo Cecchi
none Details | Review
border-image: ensure image->resolved is set when resolving the gradient (969 bytes, patch)
2011-05-27 22:14 UTC, Cosimo Cecchi
none Details | Review
docs: expand the docs for border-image (3.48 KB, patch)
2011-05-31 13:33 UTC, Cosimo Cecchi
none Details | Review

Description Cosimo Cecchi 2011-05-26 21:58:14 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.
Comment 1 Cosimo Cecchi 2011-05-26 21:58:16 UTC
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.
Comment 2 Cosimo Cecchi 2011-05-26 21:58:19 UTC
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.
Comment 3 Matthias Clasen 2011-05-27 02:24:22 UTC
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
Comment 4 Cosimo Cecchi 2011-05-27 22:07:27 UTC
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.
Comment 5 Cosimo Cecchi 2011-05-27 22:07:37 UTC
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.
Comment 6 Cosimo Cecchi 2011-05-27 22:07:47 UTC
Created attachment 188789 [details] [review]
reftests: add reftests for border-image with gradient and url sources
Comment 7 Cosimo Cecchi 2011-05-27 22:14:38 UTC
Created attachment 188790 [details] [review]
border-image: ensure image->resolved is set when resolving the gradient
Comment 8 Cosimo Cecchi 2011-05-27 22:18:30 UTC
(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.
Comment 9 Cosimo Cecchi 2011-05-31 13:33:53 UTC
Created attachment 188936 [details] [review]
docs: expand the docs for border-image
Comment 10 Matthias Clasen 2011-05-31 18:37:43 UTC
Looks good to me.
Comment 11 Benjamin Otte (Company) 2011-06-01 11:18:48 UTC
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.
Comment 12 Matthias Clasen 2011-06-01 13:38:18 UTC
This is really good information - would be great to have it in a code comment somewhere...
Comment 13 Cosimo Cecchi 2011-06-10 20:46:55 UTC
This is now fixed in master.