Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Eli Zaretskii]

Thank you for your contribution, but I noticed a few minor issues with
it:

> +@defun svg-embed-base-uri-image svg relative-filename &rest args
> +To @var{svg} add an embedded (raster) image placed at
> +@var{relative-filename}.  @var{relative-filename} is searched inside
> +@code{file-name-directory} of the @code{:base-uri} svg image property.

This is the only place in the manual where the :base-uri attribute is
mentioned.  Can we please add some minimal description of what that
attribute does and how it is useful?

> +This improves the performance of embedding large images.

What is "this" that improves the performance, and how does it do so?
We should at least hint at the improvement, without that this sentence
sounds puzzling.

> +** Can explicitly specify base_uri for svg images.

This should say

  It is now possible to explicitly specify base_uri for SVG images.

Please also add a short explanation of what "base_uri" is, or point to
some canonical description on the net.

> +':base-uri' image property can be used to explicitly specify base_uri
> +for embedded images into svg. ':base-uri' is supported for both file
> +and data svg images.        ^^^

Please leave 2 spaces between sentences, per our conventions.

> +** 'svg-embed-base-uri-image' added to embed images

Suggest to reword:

  New function 'svg-embed-base-uri-image'.

> +(defun svg-embed-base-uri-image (svg relative-filename &rest args)
> +  "Insert image placed at RELATIVE-FILENAME into the SVG structure.

"Placed" is not the word I'd use here.  "Located" is better.

> @@ -9782,10 +9786,10 @@ svg_load (struct frame *f, struct image *img)
>  	  image_error ("Invalid image data `%s'", data);
>  	  return 0;
>  	}
> -      original_filename = BVAR (current_buffer, filename);
> +      if (!STRINGP (base_uri))
> +        base_uri = BVAR (current_buffer, filename);
>        success_p = svg_load_image (f, img, SSDATA (data), SBYTES (data),
> -                                  (NILP (original_filename) ? NULL
> -				   : SSDATA (original_filename)));
> +                                  (NILP (base_uri) ? NULL : SSDATA (base_uri)));

Here you are passing an unencoded file name to svg_load_image, which
expects an encoded file name.  (Yes, the original code was wrong as
well).

Can these minor issues be fixed, please?

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 2928 bytes --]

сб, 12 дек. 2020 г. в 22:43, Eli Zaretskii <eliz@gnu.org>:

> Thank you for your contribution, but I noticed a few minor issues with
> it:
>
> > +@defun svg-embed-base-uri-image svg relative-filename &rest args
> > +To @var{svg} add an embedded (raster) image placed at
> > +@var{relative-filename}.  @var{relative-filename} is searched inside
> > +@code{file-name-directory} of the @code{:base-uri} svg image property.
>
> This is the only place in the manual where the :base-uri attribute is
> mentioned.  Can we please add some minimal description of what that
> attribute does and how it is useful?
>

This is only function, where `:base-uri` could be useful


> > +This improves the performance of embedding large images.
>
> What is "this" that improves the performance, and how does it do so?
>

"this" - having support for `:base_uri`

We should at least hint at the improvement, without that this sentence
> sounds puzzling.
>
>
I'll try my best in rewording this, however take into account that I'm very
bad at wording in English.  Maybe it is better for someone else to write
docs for it?

> +** Can explicitly specify base_uri for svg images.
>
> This should say
>
>   It is now possible to explicitly specify base_uri for SVG images.
>
> Please also add a short explanation of what "base_uri" is, or point to
> some canonical description on the net.
>
> > +':base-uri' image property can be used to explicitly specify base_uri
> > +for embedded images into svg. ':base-uri' is supported for both file
> > +and data svg images.        ^^^
>
> Please leave 2 spaces between sentences, per our conventions.
>
> > +** 'svg-embed-base-uri-image' added to embed images
>
> Suggest to reword:
>
>   New function 'svg-embed-base-uri-image'.
>
> > +(defun svg-embed-base-uri-image (svg relative-filename &rest args)
> > +  "Insert image placed at RELATIVE-FILENAME into the SVG structure.
>
> "Placed" is not the word I'd use here.  "Located" is better.
>
> > @@ -9782,10 +9786,10 @@ svg_load (struct frame *f, struct image *img)
> >         image_error ("Invalid image data `%s'", data);
> >         return 0;
> >       }
> > -      original_filename = BVAR (current_buffer, filename);
> > +      if (!STRINGP (base_uri))
> > +        base_uri = BVAR (current_buffer, filename);
> >        success_p = svg_load_image (f, img, SSDATA (data), SBYTES (data),
> > -                                  (NILP (original_filename) ? NULL
> > -                                : SSDATA (original_filename)));
> > +                                  (NILP (base_uri) ? NULL : SSDATA
> (base_uri)));
>
> Here you are passing an unencoded file name to svg_load_image, which
> expects an encoded file name.  (Yes, the original code was wrong as
> well).
>
> Can these minor issues be fixed, please?
>

I'll compose patch you want a bit later

-- 
lg

[-- Attachment #2: Type: text/html, Size: 4255 bytes --]

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Lars Ingebrigtsen]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

With this change, I get

(emacs:3509529): librsvg-CRITICAL **: 23:41:23.634: rsvg_handle_set_dpi_x_y: assertion 'is_rsvg_handle(handle)' failed

on some SVG images (example included below).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

[-- Attachment #2: a.svg --]
[-- Type: image/svg+xml, Size: 171 bytes --]

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Alan Third]

On Sat, Dec 12, 2020 at 11:43:00PM +0100, Lars Ingebrigtsen wrote:
> With this change, I get
> 
> (emacs:3509529): librsvg-CRITICAL **: 23:41:23.634: rsvg_handle_set_dpi_x_y: assertion 'is_rsvg_handle(handle)' failed
> 
> on some SVG images (example included below).

Sorry, that was my fault, caused by another commit. I put the setting
of DPI before the check for whether the SVG file loaded, so when it
failed (as that image does) it tried to use the rsvg_handle anyway.

I've moved the DPI code to after the check and the image fails
silently now, the same as before.

-- 
Alan Third

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I've moved the DPI code to after the check and the image fails
> silently now, the same as before.

Thanks; I can confirm that this makes the error message go away.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Eli Zaretskii]

> From: Evgeny Zajcev <zevlg@yandex.ru>
> Date: Sat, 12 Dec 2020 23:42:36 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> > > +@defun svg-embed-base-uri-image svg relative-filename &rest args
> > > +To @var{svg} add an embedded (raster) image placed at
> > > +@var{relative-filename}.  @var{relative-filename} is searched inside
> > > +@code{file-name-directory} of the @code{:base-uri} svg image property.
> >
> > This is the only place in the manual where the :base-uri attribute is
> > mentioned.  Can we please add some minimal description of what that
> > attribute does and how it is useful?
> 
> This is only function, where `:base-uri` could be useful

Then I think its documentation should say something about what a "base
URI" is.

> > > +This improves the performance of embedding large images.
> >
> > What is "this" that improves the performance, and how does it do so?
> >
> 
> "this" - having support for `:base_uri`

Yes, but _why_ does having the base URI improve the performance?

> We should at least hint at the improvement, without that this sentence
> > sounds puzzling.
> >
> >
> I'll try my best in rewording this, however take into account that I'm very
> bad at wording in English.  Maybe it is better for someone else to write
> docs for it?

Just explain it in your words, and I will reword if needed.

> I'll compose patch you want a bit later

Thanks in advance.

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 1663 bytes --]

вс, 13 дек. 2020 г. в 18:05, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <zevlg@yandex.ru>
> > Date: Sat, 12 Dec 2020 23:42:36 +0300
> > Cc: emacs-devel <emacs-devel@gnu.org>
> >
> > > > +@defun svg-embed-base-uri-image svg relative-filename &rest args
> > > > +To @var{svg} add an embedded (raster) image placed at
> > > > +@var{relative-filename}.  @var{relative-filename} is searched inside
> > > > +@code{file-name-directory} of the @code{:base-uri} svg image
> property.
> > >
> > > This is the only place in the manual where the :base-uri attribute is
> > > mentioned.  Can we please add some minimal description of what that
> > > attribute does and how it is useful?
> >
> > This is only function, where `:base-uri` could be useful
>
> Then I think its documentation should say something about what a "base
> URI" is.
>
> > > > +This improves the performance of embedding large images.
> > >
> > > What is "this" that improves the performance, and how does it do so?
> > >
> >
> > "this" - having support for `:base_uri`
>
> Yes, but _why_ does having the base URI improve the performance?
>
> > We should at least hint at the improvement, without that this sentence
> > > sounds puzzling.
> > >
> > >
> > I'll try my best in rewording this, however take into account that I'm
> very
> > bad at wording in English.  Maybe it is better for someone else to write
> > docs for it?
>
> Just explain it in your words, and I will reword if needed.
>
> > I'll compose patch you want a bit later
>
> Thanks in advance.
>

Here I've composed the patch.  As I can see NEWS already fixed


-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 2443 bytes --]

[-- Attachment #2: 0001-Improvements-for-base-uri-svg-image-property.patch --]
[-- Type: text/x-patch, Size: 2586 bytes --]

From 47288ad1203c7fe3b58e8bdab46c4932d37150dc Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Thu, 17 Dec 2020 01:04:09 +0300
Subject: [PATCH] Improvements for `:base-uri' svg image property

* src/image.c (svg_load): Use ENCODE_FILE for `:base-uri'

* doc/lispref/display.texi: Add more documentation for `:base-uri'
---
 doc/lispref/display.texi | 8 +++++++-
 src/image.c              | 8 +++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 2b3119ea59..2ecd14fed9 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5904,7 +5904,13 @@ SVG Images
 To @var{svg} add an embedded (raster) image placed at
 @var{relative-filename}.  @var{relative-filename} is searched inside
 @code{file-name-directory} of the @code{:base-uri} svg image property.
-This improves the performance of embedding large images.
+@code{:base-uri} specifies a (possible non-existing) filename name of
+the svg image to be created, thus all the embedded files are searched
+relatively to the @code{:base-uri} filename's directory.  If
+@code{:base-uri} is ommited, then filename from where svg image is
+loaded is used.  Using @code{:base-uri} improves the performance of
+embedding large images, comparing to @code{svg-embed}, because all the
+work is done directly by librsvg.
 
 @lisp
 ;; Embeding /tmp/subdir/rms.jpg and /tmp/another/rms.jpg
diff --git a/src/image.c b/src/image.c
index dc06e9ce20..ba39fdc785 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9779,8 +9779,9 @@ svg_load (struct frame *f, struct image *img)
 	}
       /* If the file was slurped into memory properly, parse it.  */
       if (!STRINGP (base_uri))
-        base_uri = ENCODE_FILE (file);
-      success_p = svg_load_image (f, img, contents, size, SSDATA (base_uri));
+        base_uri = file;
+      success_p = svg_load_image (f, img, contents, size,
+                                  SSDATA (ENCODE_FILE (base_uri)));
       xfree (contents);
     }
   /* Else it's not a file, it's a Lisp object.  Load the image from a
@@ -9798,7 +9799,8 @@ svg_load (struct frame *f, struct image *img)
       if (!STRINGP (base_uri))
         base_uri = BVAR (current_buffer, filename);
       success_p = svg_load_image (f, img, SSDATA (data), SBYTES (data),
-                                  (NILP (base_uri) ? NULL : SSDATA (base_uri)));
+                                  (STRINGP (base_uri) ?
+                                   SSDATA (ENCODE_FILE (base_uri)) : NULL));
     }
 
   return success_p;
-- 
2.25.1

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Eli Zaretskii]

> From: Evgeny Zajcev <zevlg@yandex.ru>
> Date: Thu, 17 Dec 2020 01:08:12 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Here I've composed the patch.  As I can see NEWS already fixed

Do you think the current NEWS entry is sufficiently complete and
accurate?

The patch LGTM, with a couple of wording nits:

> * doc/lispref/display.texi: Add more documentation for `:base-uri'

This should mention the name of the @node where the change is done, in
parentheses (as if it were a function).

> diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> index 2b3119ea59..2ecd14fed9 100644
> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -5904,7 +5904,13 @@ SVG Images
>  To @var{svg} add an embedded (raster) image placed at
>  @var{relative-filename}.  @var{relative-filename} is searched inside
>  @code{file-name-directory} of the @code{:base-uri} svg image property.
> -This improves the performance of embedding large images.
> +@code{:base-uri} specifies a (possible non-existing) filename name of
                                 ^^^^^^^^               ^^^^^^^^^^^^^
"possibly" and "file name".

Thanks.

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 1365 bytes --]

сб, 19 дек. 2020 г. в 13:09, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <zevlg@yandex.ru>
> > Date: Thu, 17 Dec 2020 01:08:12 +0300
> > Cc: emacs-devel <emacs-devel@gnu.org>
> >
> > Here I've composed the patch.  As I can see NEWS already fixed
>
> Do you think the current NEWS entry is sufficiently complete and
> accurate?
>

Yeah, NEWS entries are good

The patch LGTM, with a couple of wording nits:
>
> > * doc/lispref/display.texi: Add more documentation for `:base-uri'
>
> This should mention the name of the @node where the change is done, in
> parentheses (as if it were a function).
>
> > diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> > index 2b3119ea59..2ecd14fed9 100644
> > --- a/doc/lispref/display.texi
> > +++ b/doc/lispref/display.texi
> > @@ -5904,7 +5904,13 @@ SVG Images
> >  To @var{svg} add an embedded (raster) image placed at
> >  @var{relative-filename}.  @var{relative-filename} is searched inside
> >  @code{file-name-directory} of the @code{:base-uri} svg image property.
> > -This improves the performance of embedding large images.
> > +@code{:base-uri} specifies a (possible non-existing) filename name of
>                                  ^^^^^^^^               ^^^^^^^^^^^^^
> "possibly" and "file name".
>
>
Noted, here is the updated patch

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 2194 bytes --]

[-- Attachment #2: 0001-Improvements-for-base-uri-svg-image-property.patch --]
[-- Type: text/x-patch, Size: 2597 bytes --]

From 224ad15336f00df22609c28506ea231c2a68857d Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Thu, 17 Dec 2020 01:04:09 +0300
Subject: [PATCH] Improvements for `:base-uri' svg image property

* src/image.c (svg_load): Use ENCODE_FILE for `:base-uri'

* doc/lispref/display.texi (SVG Images): Add more documentation for
  `:base-uri'
---
 doc/lispref/display.texi | 8 +++++++-
 src/image.c              | 8 +++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 2b3119ea59..949fd8987c 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5904,7 +5904,13 @@ SVG Images
 To @var{svg} add an embedded (raster) image placed at
 @var{relative-filename}.  @var{relative-filename} is searched inside
 @code{file-name-directory} of the @code{:base-uri} svg image property.
-This improves the performance of embedding large images.
+@code{:base-uri} specifies a (possibly non-existing) file name of the
+svg image to be created, thus all the embedded files are searched
+relatively to the @code{:base-uri} filename's directory.  If
+@code{:base-uri} is ommited, then filename from where svg image is
+loaded is used.  Using @code{:base-uri} improves the performance of
+embedding large images, comparing to @code{svg-embed}, because all the
+work is done directly by librsvg.
 
 @lisp
 ;; Embeding /tmp/subdir/rms.jpg and /tmp/another/rms.jpg
diff --git a/src/image.c b/src/image.c
index dc06e9ce20..ba39fdc785 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9779,8 +9779,9 @@ svg_load (struct frame *f, struct image *img)
 	}
       /* If the file was slurped into memory properly, parse it.  */
       if (!STRINGP (base_uri))
-        base_uri = ENCODE_FILE (file);
-      success_p = svg_load_image (f, img, contents, size, SSDATA (base_uri));
+        base_uri = file;
+      success_p = svg_load_image (f, img, contents, size,
+                                  SSDATA (ENCODE_FILE (base_uri)));
       xfree (contents);
     }
   /* Else it's not a file, it's a Lisp object.  Load the image from a
@@ -9798,7 +9799,8 @@ svg_load (struct frame *f, struct image *img)
       if (!STRINGP (base_uri))
         base_uri = BVAR (current_buffer, filename);
       success_p = svg_load_image (f, img, SSDATA (data), SBYTES (data),
-                                  (NILP (base_uri) ? NULL : SSDATA (base_uri)));
+                                  (STRINGP (base_uri) ?
+                                   SSDATA (ENCODE_FILE (base_uri)) : NULL));
     }
 
   return success_p;
-- 
2.25.1

Re: master f45ce78 2/2: Explicitly specify svg base_uri using `:base-uri' image property

[Eli Zaretskii]

> From: Evgeny Zajcev <zevlg@yandex.ru>
> Date: Sat, 19 Dec 2020 15:24:38 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
>  Do you think the current NEWS entry is sufficiently complete and
>  accurate?
> 
> Yeah, NEWS entries are good
> 
>  The patch LGTM, with a couple of wording nits:
> 
>  > * doc/lispref/display.texi: Add more documentation for `:base-uri'
> 
>  This should mention the name of the @node where the change is done, in
>  parentheses (as if it were a function).
> 
>  > diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
>  > index 2b3119ea59..2ecd14fed9 100644
>  > --- a/doc/lispref/display.texi
>  > +++ b/doc/lispref/display.texi
>  > @@ -5904,7 +5904,13 @@ SVG Images
>  >  To @var{svg} add an embedded (raster) image placed at
>  >  @var{relative-filename}.  @var{relative-filename} is searched inside
>  >  @code{file-name-directory} of the @code{:base-uri} svg image property.
>  > -This improves the performance of embedding large images.
>  > +@code{:base-uri} specifies a (possible non-existing) filename name of
>                                   ^^^^^^^^               ^^^^^^^^^^^^^
>  "possibly" and "file name".
> 
> Noted, here is the updated patch 

Thanks, pushed to the master branch.