GNOME Bugzilla – Bug 663472
Support Opacity for SolidColorBrush elements
Last modified: 2011-11-12 01:54:20 UTC
Created attachment 200781 [details] [review] adds Opacity support for SolidColorBrush elements Included patch adds Opacity support for SolidColorBrush elements. Parsing of SolidColorBrush attributes is rearranged because Color and Opacity attributes could be in any order and to simplify error checking.
Created attachment 200782 [details] example XPS document showing SolidColorBrush with opacity
Review of attachment 200781 [details] [review]: Thanks for the patch, I have some small comments. ::: libgxps/gxps-page.c @@ +1277,3 @@ if (strcmp (element_name, "SolidColorBrush") == 0) { + gdouble a, r, g, b; + const gchar *colorstr = NULL; Use color_str instead. @@ +1285,2 @@ } else if (strcmp (names[i], "Opacity") == 0) { + brush->opacity = g_ascii_strtod (values[i], NULL); Use gxps_value_get_double() and fill error if returns FALSE. @@ +1290,3 @@ } + + if (colorstr && gxps_color_parse (colorstr, &a, &r, &g, &b)) { Instead of this, we could modify gxps_create_solid_color_pattern() to receive a global alpha value or add a new method gxps_create_solid_color_pattern_with_alpha(). @@ +1293,3 @@ + GXPS_DEBUG (g_message ("set_fill_pattern (solid)")); + a *= brush->opacity; + brush->pattern = (a != 1.0) ? I've realized we don't need this, because cairo_pattern_create_rgb() calls cairo_pattern_create_rgba() with alpha = 1, so we can just use cairo_pattern_create_rgba() always.
Created attachment 201043 [details] [review] adds Opacity support for SolidColorBrush elements
Review of attachment 201043 [details] [review]: Great!, I've just pushed it to git master, with the modifications I comment below. ::: libgxps/gxps-page.c @@ +975,3 @@ return NULL; + a *= alpha; a i not used after this, so we can just call pattern = cairo_pattern_create_rgba (r, g, b, a * alpha); @@ +1448,3 @@ brush->ctx->page->priv->source, G_MARKUP_ERROR_INVALID_CONTENT, + "Opacity", "SolidColorBrush", I don't know why you changed it, because it was correct, it's element_name, attribute_name @@ +1457,3 @@ } + + if (color_str) { Color is required, so we should fail with MISSING_ATTRIBUTE error if color is missing. @@ +1466,3 @@ + brush->ctx->page->priv->source, + G_MARKUP_ERROR_INVALID_CONTENT, + "Color", "SolidColorBrush", Same here, it's first the element_name
(In reply to comment #4) > @@ +1448,3 @@ > brush->ctx->page->priv->source, > G_MARKUP_ERROR_INVALID_CONTENT, > + "Opacity", "SolidColorBrush", > > I don't know why you changed it, because it was correct, it's element_name, > attribute_name I was going by the error messages I received from my test files. Example: Error rendering page /Documents/1/Pages/1.fpage: /Documents/1/Pages/1.fpage:4:55 invalid content in attribute 'SolidColorBrush' of element 'Opacity': abc See new bug https://bugzilla.gnome.org/show_bug.cgi?id=663899