GNU bug report logs - #75712
HAVE_RSVG: svg_css_length_to_pixels doesn't handle RSVG_UNIT_CH

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 75712 in the body.
You can then email your comments to 75712 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: HAVE_RSVG: svg_css_length_to_pixels doesn't handle RSVG_UNIT_CH
Date: Mon, 20 Jan 2025 14:26:22 -0600

In Bug#75451, Pip Cet <pipcet <at> protonmail.com> writes:

> svg_css_length_to_pixels doesn't handle the RSVG_UNIT_CH case.  I don't
> know at which version rsvg started to offer this unit; it's not
> indicated in the header file.

It seems to have been added in 2.58:

https://gnome.pages.gitlab.gnome.org/librsvg/Rsvg-2.0/enum.Unit.html

Message #8 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: 75712 <at> debbugs.gnu.org
Cc: Pip Cet <pipcet <at> protonmail.com>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Mon, 20 Jan 2025 20:25:18 -0600

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> In Bug#75451, Pip Cet <pipcet <at> protonmail.com> writes:
>
>> svg_css_length_to_pixels doesn't handle the RSVG_UNIT_CH case.  I don't
>> know at which version rsvg started to offer this unit; it's not
>> indicated in the header file.
>
> It seems to have been added in 2.58:
>
> https://gnome.pages.gitlab.gnome.org/librsvg/Rsvg-2.0/enum.Unit.html

I came up with the following patch, but it's not ideal.

With CSS 3, "the ch unit falls back to 0.5em in the general case, and to
1em when it would be typeset upright".
https://www.w3.org/TR/css-values-3/#ch

Unfortunately, this patch doesn't take into account the "direction" or
"writing-mode" CSS attributes, which is what seems to specify the above.
I couldn't find a way to easily get these CSS attributes using librsvg.

Instead, I just naively assume that they are set to their default
non-vertical value, and thus use 0.5em.  That's not always going to
work, obviously.

While the "ch" attribute seems to be rarely used, it would be nice to
get this detail right.  I'm just not sure how to do it.  Maybe look
through the CSS by hand, searching for these strings and then parsing it
out?  But that's likely to be brittle, ugly, and hard to maintain.

Anyone have any good ideas here?  It's my first time fiddling with
librsvg, so it's quite possible that I'm overlooking something.

That said, I guess this could be installed as a stopgap, to have a
better result than we have now for the common case.

[0001-Support-SVG-ch-length-values.patch (text/x-patch, attachment)]

Message #11 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75712 <at> debbugs.gnu.org
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 03:14:41 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>> In Bug#75451, Pip Cet <pipcet <at> protonmail.com> writes:
>>
>>> svg_css_length_to_pixels doesn't handle the RSVG_UNIT_CH case.  I don't
>>> know at which version rsvg started to offer this unit; it's not
>>> indicated in the header file.
>>
>> It seems to have been added in 2.58:
>>
>> https://gnome.pages.gitlab.gnome.org/librsvg/Rsvg-2.0/enum.Unit.html
>
> I came up with the following patch, but it's not ideal.

I think it will do for the case of that single unit, but I was hoping
for an improvement for what used to be the default case: we should
probably error in the case of an unknown unit, or at least print a
warning.  I was assuming RSVG would provide a simple interface to get
the string for a unit enum value, so we could print a meaningful
message, but it seems an int is all we get.

The header comment (which doesn't include the version numbers, thanks
for providing a link to those!) reads:

 * If you test for the values of this enum, please note that librsvg may add other units in the future
 * as its support for CSS improves.  Please make your code handle unknown units gracefully (e.g. with
 * a `default` case in a `switch()` statement).

But I have no idea how to do that given the API defined by that file.

As this is a fall-back case of a fall-back case, it may not be worth
spending too much time on: set a flag to let the code after the switch
statement know we've failed, print the integer value, error.

> Unfortunately, this patch doesn't take into account the "direction" or
> "writing-mode" CSS attributes, which is what seems to specify the above.
> I couldn't find a way to easily get these CSS attributes using librsvg.
>
> Instead, I just naively assume that they are set to their default
> non-vertical value, and thus use 0.5em.  That's not always going to
> work, obviously.
>
> While the "ch" attribute seems to be rarely used, it would be nice to
> get this detail right.  I'm just not sure how to do it.  Maybe look
> through the CSS by hand, searching for these strings and then parsing it
> out?  But that's likely to be brittle, ugly, and hard to maintain.

I assumed this would be useful for SVG-based fonts, so we should
probably make it depend on the Emacs environment rather than trying to
derive it from the CSS.

> Anyone have any good ideas here?  It's my first time fiddling with
> librsvg, so it's quite possible that I'm overlooking something.

I'm afraid I can't really help.  If I'd looked at the context of this
bug more closely, maybe I would have ignored it.

Pip

Message #14 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75712 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 15:59:16 +0200

> Cc: Pip Cet <pipcet <at> protonmail.com>
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Mon, 20 Jan 2025 20:25:18 -0600
> 
> --- a/src/image.c
> +++ b/src/image.c
> @@ -12008,9 +12008,15 @@ svg_css_length_to_pixels (RsvgLength length, double dpi, int font_size)
>  	 what size we make the image.  */
>        value = 0;
>        break;
> -    default:
> -      /* We should never reach this.  */
> -      value = 0;
> +#if LIBRSVG_CHECK_VERSION (2, 58, 0)
> +    case RSVG_UNIT_CH:
> +      /* FIXME: With CSS 3, "the ch unit falls back to 0.5em in the
> +	 general case, and to 1em when it would be typeset upright".
> +	 However, I could not find a way to easily get the relevant CSS
> +	 attributes using librsvg.  Thus, we simply wrongly assume the
> +	 general case is always true here.  See Bug#75712.  */
> +      value = value * font_size / 2.0;
> +#endif

This makes the code worse for librsvg versions before 2.58.  Why not
keep the old code for older versions?

Also, pointing to bug#75712 is not useful here: this bug had accrued
too many completely unrelated messages, so anyone who'd like to have
more details about this issue will curse us.  How about explaining the
main issues here in this very comment instead?

Thanks.

Message #17 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75712 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 15:04:33 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Cc: Pip Cet <pipcet <at> protonmail.com>
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Mon, 20 Jan 2025 20:25:18 -0600
>>
>> --- a/src/image.c
>> +++ b/src/image.c
>> @@ -12008,9 +12008,15 @@ svg_css_length_to_pixels (RsvgLength length, double dpi, int font_size)
>>  	 what size we make the image.  */
>>        value = 0;
>>        break;
>> -    default:
>> -      /* We should never reach this.  */
>> -      value = 0;
>> +#if LIBRSVG_CHECK_VERSION (2, 58, 0)
>> +    case RSVG_UNIT_CH:
>> +      /* FIXME: With CSS 3, "the ch unit falls back to 0.5em in the
>> +	 general case, and to 1em when it would be typeset upright".
>> +	 However, I could not find a way to easily get the relevant CSS
>> +	 attributes using librsvg.  Thus, we simply wrongly assume the
>> +	 general case is always true here.  See Bug#75712.  */
>> +      value = value * font_size / 2.0;
>> +#endif
>
> This makes the code worse for librsvg versions before 2.58.  Why not
> keep the old code for older versions?

If the librsvg library and the librsvg headers are both old, this code
is never reached.

If the headers were old but the library has been updated, I agree that
the change in behavior is incorrect.  Returning 0 means we can't do
anything with the unit, which is true.

Here's a proposed patch which:
1. prints a warning about this situation (in case the library changes)
2. omits the default label so GCC also warns, even without -Wswitch-enum
(in case the headers change)
3. includes too much commentary rather than too little.

diff --git a/src/image.c b/src/image.c
index b8405d81111..28d55a575ee 100644
--- a/src/image.c
+++ b/src/image.c
@@ -11965,34 +11965,27 @@ svg_css_length_to_pixels (RsvgLength length, double dpi, int font_size)
     {
     case RSVG_UNIT_PX:
       /* Already a pixel value.  */
-      break;
+      return value;
     case RSVG_UNIT_CM:
       /* 2.54 cm in an inch.  */
-      value = dpi * value / 2.54;
-      break;
+      return dpi * value / 2.54;
     case RSVG_UNIT_MM:
       /* 25.4 mm in an inch.  */
-      value = dpi * value / 25.4;
-      break;
+      return dpi * value / 25.4;
     case RSVG_UNIT_PT:
       /* 72 points in an inch.  */
-      value = dpi * value / 72;
-      break;
+      return dpi * value / 72;
     case RSVG_UNIT_PC:
       /* 6 picas in an inch.  */
-      value = dpi * value / 6;
-      break;
+      return dpi * value / 6;
     case RSVG_UNIT_IN:
-      value *= dpi;
-      break;
+      return value * dpi;
     case RSVG_UNIT_EM:
-      value *= font_size;
-      break;
+      return value * font_size;
     case RSVG_UNIT_EX:
       /* librsvg uses an ex height of half the em height, so we match
 	 that here.  */
-      value = value * font_size / 2.0;
-      break;
+      return value * font_size / 2.0;
     case RSVG_UNIT_PERCENT:
       /* Percent is a ratio of the containing "viewport".  We don't
 	 have a viewport, as such, as we try to draw the image to it's
@@ -12006,14 +11999,27 @@ svg_css_length_to_pixels (RsvgLength length, double dpi, int font_size)
 	 spec, this will work out correctly as librsvg will still
 	 honor the percentage sizes in its final rendering no matter
 	 what size we make the image.  */
-      value = 0;
-      break;
-    default:
-      /* We should never reach this.  */
-      value = 0;
-    }
-
-  return value;
+      return 0;
+#if LIBRSVG_CHECK_VERSION (2, 58, 0)
+    case RSVG_UNIT_CH:
+      /* FIXME: With CSS 3, "the ch unit falls back to 0.5em in the
+	 general case, and to 1em when it would be typeset upright".
+	 However, I could not find a way to easily get the relevant CSS
+	 attributes using librsvg.  Thus, we simply wrongly assume the
+	 general case is always true here.  See Bug#75712.  */
+      return value * font_size / 2.0;
+#endif
+    }
+
+  /* The rsvg header files say that more values may be added to this
+     enum, but there doesn't appear to be a way to get a string
+     representation of the new enum value.  The unfortunate
+     consequence is that the only thing we can do is to report the
+     numeric value.  */
+  image_error ("Unknown RSVG unit, code: %d", (int) length.unit);
+  /* Return 0; this special value indicates that another method of
+     obtaining the image size must be used.  */
+  return 0;
 }
 #endif
 
> Also, pointing to bug#75712 is not useful here: this bug had accrued
> too many completely unrelated messages, so anyone who'd like to have

Wrong bug.  This is bug#75712.

Pip

Message #20 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75712 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 17:18:14 +0200

> Date: Tue, 21 Jan 2025 15:04:33 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75712 <at> debbugs.gnu.org
> 
> Here's a proposed patch which:
> 1. prints a warning about this situation (in case the library changes)
> 2. omits the default label so GCC also warns, even without -Wswitch-enum
> (in case the headers change)
> 3. includes too much commentary rather than too little.

LGTM, thanks.  But please wait for Stefan to comment.

Message #23 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Pip Cet <pipcet <at> protonmail.com>
Cc: 75712 <at> debbugs.gnu.org
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 10:29:39 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Tue, 21 Jan 2025 15:04:33 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75712 <at> debbugs.gnu.org
>>
>> Here's a proposed patch which:
>> 1. prints a warning about this situation (in case the library changes)
>> 2. omits the default label so GCC also warns, even without -Wswitch-enum
>> (in case the headers change)
>> 3. includes too much commentary rather than too little.
>
> LGTM, thanks.  But please wait for Stefan to comment.

LGTM, please install.

Message #28 received at 75712-done <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 75712-done <at> debbugs.gnu.org
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 19:12:40 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> Date: Tue, 21 Jan 2025 15:04:33 +0000
>>> From: Pip Cet <pipcet <at> protonmail.com>
>>> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75712 <at> debbugs.gnu.org
>>>
>>> Here's a proposed patch which:
>>> 1. prints a warning about this situation (in case the library changes)
>>> 2. omits the default label so GCC also warns, even without -Wswitch-enum
>>> (in case the headers change)
>>> 3. includes too much commentary rather than too little.
>>
>> LGTM, thanks.  But please wait for Stefan to comment.
>
> LGTM, please install.

Pushed, closing.  Minor change compared to the patch I posted:

PSA: image_error takes Lisp_Object arguments, not integers.  Something
for your (ever-growing, no doubt) TODO list, maybe?

Pip

Message #31 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: 75712 <at> debbugs.gnu.org
Cc: pipcet <at> protonmail.com, stefankangas <at> gmail.com
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 20:24:05 +0100

Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:

> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> Date: Tue, 21 Jan 2025 15:04:33 +0000
>>>> From: Pip Cet <pipcet <at> protonmail.com>
>>>> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75712 <at> debbugs.gnu.org
>>>>
>>>> Here's a proposed patch which:
>>>> 1. prints a warning about this situation (in case the library changes)
>>>> 2. omits the default label so GCC also warns, even without -Wswitch-enum
>>>> (in case the headers change)
>>>> 3. includes too much commentary rather than too little.
>>>
>>> LGTM, thanks.  But please wait for Stefan to comment.
>>
>> LGTM, please install.
>
> Pushed, closing.  Minor change compared to the patch I posted:
>
> PSA: image_error takes Lisp_Object arguments, not integers.  Something
> for your (ever-growing, no doubt) TODO list, maybe?
>
> Pip

Maybe something --enable-check-lisp-object-type would be good for.

Message #34 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75712 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 12:02:42 -0800

Pip Cet <pipcet <at> protonmail.com> writes:

> PSA: image_error takes Lisp_Object arguments, not integers.  Something
> for your (ever-growing, no doubt) TODO list, maybe?

Sorry, you lost me.  What do you propose here?

image_error should accept things like an image.spec, so I don't see how
it could be changed to accept integers.

Message #37 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75712 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 22:49:13 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> PSA: image_error takes Lisp_Object arguments, not integers.  Something
>> for your (ever-growing, no doubt) TODO list, maybe?
>
> Sorry, you lost me.  What do you propose here?

I was proposing type safety, thinking there might be a better way than
replacing image_error and add_to_log by macros.  Didn't find one, did
the macro thing, didn't find any bugs, so probably not worth a commit.

I kind of think tiff_handler should use image_error, not add_to_log, but
the two have been equivalent forever.

> image_error should accept things like an image.spec, so I don't see how
> it could be changed to accept integers.

While we can mix and match Lisp_Objects and POD C types freely in
varargs situations, it seems, all the symbols would clash (I thought %S
was safe, but doprnt.c makes it an alias for %s), and there's little
point anyway since using nonstandard format specifiers breaks
ATTRIBUTE_FORMAT_PRINTF.

So no real proposal left here.  listn is a strange animal and we should
probably use Flist instead, but that should wait until purespace is
gone, so no patch there, either.

Pip

Message #40 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75712 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Tue, 21 Jan 2025 17:40:05 -0600

Pip Cet <pipcet <at> protonmail.com> writes:

> I kind of think tiff_handler should use image_error, not add_to_log, but
> the two have been equivalent forever.

I think it'd be worth changing that one call, if only to be fully
locally consistent in image.c.

> listn is a strange animal and we should probably use Flist instead,
> but that should wait until purespace is gone,

Yes, it seems like yet another post-"scratch/no-purespace merge" cleanup.

Message #43 received at 75712 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75712 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#75712: HAVE_RSVG: svg_css_length_to_pixels doesn't handle
 RSVG_UNIT_CH
Date: Wed, 22 Jan 2025 14:59:19 -0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> I kind of think tiff_handler should use image_error, not add_to_log, but
>> the two have been equivalent forever.
>
> I think it'd be worth changing that one call, if only to be fully
> locally consistent in image.c.

Now done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.