Displaying remote images

Displaying remote images

[Jack Kamm]

When trying to display a remote image file in an org-mode buffer, I
only see a blank square instead of the image.

This is a longstanding problem, and there was an attempt to patch it in
2014, but the patch was never accepted:
https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html

The fault might be with image.el rather than with org-mode itself --
for example, when I execute the following elisp, I get the same blank
box:

(insert-image (create-image "/scp:pi:/home/pi/foo.png"))

However, if I open the remote file in its own buffer using image-mode,
I can correctly view the image.

Anyways, I would like to try and fix this, but not sure whether we
should pursue a fix in org.el (as in the linked patch), or in
image.el, or elsewhere.

Any advice on how to proceed here?

Re: Displaying remote images

[briangpowell .]

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

* Idea 1
Create a "cron job" or "at job" that does updates up to the minute and make
those images current...and local--and the cron job can auto-delete the temp
file later or back it up

* Idea 2
Make an elisp .el program that gets loaded and run whenever you open the
.org file with the remote image...and that code updates the images locally

* Idea 3
Use the patch you reference and create your own .el program that gets
loaded and run when you want it--a cursory examination of the patch
reveals that it depends on ImageMagick which is great software that
everyone should use; but, it's a dependency that is NOT emacs software and
so it makes sense that it was never put into the main OrgMode software tree

P.S. I created code and methods to put and run animated .gif's into OrgMode
buffers and run them, it was amusing; but, it wasted a lot of ram and
slowed everything down

Emacs and OrgMode do many great things; but, the focus has always been, at
it's core, the most powerful editor and stateless organizing software--it's
not a browser of dynamically generated Internet pics--Emacs W3 was created
to do that, and it hasn't been updated in years--suggest you look at Emacs
W3 browser code and/or w3m browser or even UZBL browser--all of which were
made into good extensions for doing such things--UZBL by the way has been
made to operate fully encapsulated in an emacs buffer





On Thu, Nov 28, 2019 at 12:36 PM Jack Kamm <jackkamm@gmail.com> wrote:

> When trying to display a remote image file in an org-mode buffer, I
> only see a blank square instead of the image.
>
> This is a longstanding problem, and there was an attempt to patch it in
> 2014, but the patch was never accepted:
> https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html
>
> The fault might be with image.el rather than with org-mode itself --
> for example, when I execute the following elisp, I get the same blank
> box:
>
> (insert-image (create-image "/scp:pi:/home/pi/foo.png"))
>
> However, if I open the remote file in its own buffer using image-mode,
> I can correctly view the image.
>
> Anyways, I would like to try and fix this, but not sure whether we
> should pursue a fix in org.el (as in the linked patch), or in
> image.el, or elsewhere.
>
> Any advice on how to proceed here?
>
>

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

Re: Displaying remote images

[Jack Kamm]

Hi Brian,

> Emacs and OrgMode do many great things; but, the focus has always been, at
> it's core, the most powerful editor and stateless organizing software--it's
> not a browser of dynamically generated Internet pics--Emacs W3 was created
> to do that, and it hasn't been updated in years--suggest you look at Emacs
> W3 browser code and/or w3m browser or even UZBL browser--all of which were
> made into good extensions for doing such things--UZBL by the way has been
> made to operate fully encapsulated in an emacs buffer

I'm not looking for a browser of dynamically generated Internet pics --
I would just like to display a static image that lives on an SSH server
somewhere.

In particular, my use-case is for making plots with a remote R session
via org-babel. I want those plots to display correctly in the org-file
I'm working in (which may also be on the remote server).

> * Idea 3
> Use the patch you reference and create your own .el program that gets
> loaded and run when you want it--a cursory examination of the patch
> reveals that it depends on ImageMagick which is great software that
> everyone should use; but, it's a dependency that is NOT emacs software and
> so it makes sense that it was never put into the main OrgMode software tree

The patch does not add any dependency on ImageMagick -- those references
to imagemagick existed before the patch, and are still in org.el today.

> * Idea 1
> Create a "cron job" or "at job" that does updates up to the minute and make
> those images current...and local--and the cron job can auto-delete the temp
> file later or back it up
>
> * Idea 2
> Make an elisp .el program that gets loaded and run whenever you open the
> .org file with the remote image...and that code updates the images locally

These are interesting ideas, but I'm only interested in looking at
static images, so they seem a bit like overkill for my desired use case.

Re: Displaying remote images

[Jack Kamm]

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

I've attached a patch which implements displaying remote images.

> This is a longstanding problem, and there was an attempt to patch it in
> 2014, but the patch was never accepted:
> https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html

Compared to the previous attempt from 2014, I think my patch is simpler
-- it doesn't require creating any temp files.

> The fault might be with image.el rather than with org-mode itself --
> for example, when I execute the following elisp, I get the same blank
> box:

After doing some reading, I learned that image.el doesn't really create
the image. Instead, create-image simply creates a blank string with a
text property pointing to the image file location, and the rendering of
the image gets handled later by the C code (for example, png_load_body()
in image.c), which doesn't know how to read remote image files.

Since I wasn't comfortable trying to get the C code to read the remote
file, I instead took the approach used by image-mode.el, which reads the
remote image file and passes its contents directly to create-image,
instead of just passing the filename.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-display-remote-images.patch --]
[-- Type: text/x-patch, Size: 1237 bytes --]

From 47120666dad6eb0b6ca716325d7de86924e1d28e Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Thu, 28 Nov 2019 17:45:56 -0800
Subject: [PATCH] org: display remote images

---
 lisp/org.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 90f222c8b..dc7bcc7aa 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16754,13 +16754,20 @@ buffer boundaries with possible narrowing."
 			    (t nil)))
 			  (old (get-char-property-and-overlay
 				(org-element-property :begin link)
-				'org-image-overlay)))
+				'org-image-overlay))
+			  (remote-p (file-remote-p file)))
 		      (if (and (car-safe old) refresh)
 			  (image-refresh (overlay-get (cdr old) 'display))
-			(let ((image (create-image file
+			(let ((image (create-image (if (not remote-p)
+						       file
+						     (with-temp-buffer
+						       (insert-file-contents file)
+						       (string-make-unibyte
+							(buffer-substring-no-properties
+							 (point-min) (point-max)))))
 						   (and (image-type-available-p 'imagemagick)
 							width 'imagemagick)
-						   nil
+						   remote-p
 						   :width width)))
 			  (when image
 			    (let ((ov (make-overlay
-- 
2.24.0

Re: Displaying remote images

[briangpowell .]

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

* Great, see a lot of interest and use of image-mode.el, suggest you check
out this thread

https://emacs.stackexchange.com/questions/3432/display-images-in-eshell-with-iimage-mode

** Suggest you check out and use iimage-mode.el --it may help ...that's
iimage-mode.el rather than, or in addition to, your use of image-mode.el



On Thu, Nov 28, 2019 at 9:20 PM Jack Kamm <jackkamm@gmail.com> wrote:

> I've attached a patch which implements displaying remote images.
>
> > This is a longstanding problem, and there was an attempt to patch it in
> > 2014, but the patch was never accepted:
> > https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html
>
> Compared to the previous attempt from 2014, I think my patch is simpler
> -- it doesn't require creating any temp files.
>
> > The fault might be with image.el rather than with org-mode itself --
> > for example, when I execute the following elisp, I get the same blank
> > box:
>
> After doing some reading, I learned that image.el doesn't really create
> the image. Instead, create-image simply creates a blank string with a
> text property pointing to the image file location, and the rendering of
> the image gets handled later by the C code (for example, png_load_body()
> in image.c), which doesn't know how to read remote image files.
>
> Since I wasn't comfortable trying to get the C code to read the remote
> file, I instead took the approach used by image-mode.el, which reads the
> remote image file and passes its contents directly to create-image,
> instead of just passing the filename.
>
>

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

Re: Displaying remote images

[Nick Dokos]

Jack Kamm <jackkamm@gmail.com> writes:

> I've attached a patch which implements displaying remote images.
>
>> This is a longstanding problem, and there was an attempt to patch it in
>> 2014, but the patch was never accepted:
>> https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html
>
> Compared to the previous attempt from 2014, I think my patch is simpler
> -- it doesn't require creating any temp files.
>
>> The fault might be with image.el rather than with org-mode itself --
>> for example, when I execute the following elisp, I get the same blank
>> box:
>
> After doing some reading, I learned that image.el doesn't really create
> the image. Instead, create-image simply creates a blank string with a
> text property pointing to the image file location, and the rendering of
> the image gets handled later by the C code (for example, png_load_body()
> in image.c), which doesn't know how to read remote image files.
>
> Since I wasn't comfortable trying to get the C code to read the remote
> file, I instead took the approach used by image-mode.el, which reads the
> remote image file and passes its contents directly to create-image,
> instead of just passing the filename.
>
> From 47120666dad6eb0b6ca716325d7de86924e1d28e Mon Sep 17 00:00:00 2001
> From: Jack Kamm <jackkamm@gmail.com>
> Date: Thu, 28 Nov 2019 17:45:56 -0800
> Subject: [PATCH] org: display remote images
>
> ---
>  lisp/org.el | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 90f222c8b..dc7bcc7aa 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -16754,13 +16754,20 @@ buffer boundaries with possible narrowing."
>  			    (t nil)))
>  			  (old (get-char-property-and-overlay
>  				(org-element-property :begin link)
> -				'org-image-overlay)))
> +				'org-image-overlay))
> +			  (remote-p (file-remote-p file)))
>  		      (if (and (car-safe old) refresh)
>  			  (image-refresh (overlay-get (cdr old) 'display))
> -			(let ((image (create-image file
> +			(let ((image (create-image (if (not remote-p)
> +						       file
> +						     (with-temp-buffer
> +						       (insert-file-contents file)
> +						       (string-make-unibyte
> +							(buffer-substring-no-properties
> +							 (point-min) (point-max)))))
>  						   (and (image-type-available-p 'imagemagick)
>  							width 'imagemagick)
> -						   nil
> +						   remote-p
>  						   :width width)))
>  			  (when image
>  			    (let ((ov (make-overlay

FWIW, looks good to me, but I've only (carefully) read the patch: I have not actually ran it.

-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler

Re: Displaying remote images

[briangpowell .]

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

* As always I much agree with Nick, looks like a great patch

** Meanwhile, this will read your R output and stick it at the end of the
line & and show it all-at-once

elisp:(progn (shell-command "rsync -a BlahRemoteHost:/blah-R-output.png
/tmp")(sleep-for 3)(iimage-mode))]] /tmp/blah-R-output.png <-[The .png file
showed up right here--no brackets necessary--but name of the file is]

*** RSync may be replaced by SCP for sure


On Mon, Dec 2, 2019 at 3:29 PM Nick Dokos <ndokos@gmail.com> wrote:

> Jack Kamm <jackkamm@gmail.com> writes:
>
> > I've attached a patch which implements displaying remote images.
> >
> >> This is a longstanding problem, and there was an attempt to patch it in
> >> 2014, but the patch was never accepted:
> >> https://lists.gnu.org/archive/html/emacs-orgmode/2014-11/msg00583.html
> >
> > Compared to the previous attempt from 2014, I think my patch is simpler
> > -- it doesn't require creating any temp files.
> >
> >> The fault might be with image.el rather than with org-mode itself --
> >> for example, when I execute the following elisp, I get the same blank
> >> box:
> >
> > After doing some reading, I learned that image.el doesn't really create
> > the image. Instead, create-image simply creates a blank string with a
> > text property pointing to the image file location, and the rendering of
> > the image gets handled later by the C code (for example, png_load_body()
> > in image.c), which doesn't know how to read remote image files.
> >
> > Since I wasn't comfortable trying to get the C code to read the remote
> > file, I instead took the approach used by image-mode.el, which reads the
> > remote image file and passes its contents directly to create-image,
> > instead of just passing the filename.
> >
> > From 47120666dad6eb0b6ca716325d7de86924e1d28e Mon Sep 17 00:00:00 2001
> > From: Jack Kamm <jackkamm@gmail.com>
> > Date: Thu, 28 Nov 2019 17:45:56 -0800
> > Subject: [PATCH] org: display remote images
> >
> > ---
> >  lisp/org.el | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/lisp/org.el b/lisp/org.el
> > index 90f222c8b..dc7bcc7aa 100644
> > --- a/lisp/org.el
> > +++ b/lisp/org.el
> > @@ -16754,13 +16754,20 @@ buffer boundaries with possible narrowing."
> >                           (t nil)))
> >                         (old (get-char-property-and-overlay
> >                               (org-element-property :begin link)
> > -                             'org-image-overlay)))
> > +                             'org-image-overlay))
> > +                       (remote-p (file-remote-p file)))
> >                     (if (and (car-safe old) refresh)
> >                         (image-refresh (overlay-get (cdr old) 'display))
> > -                     (let ((image (create-image file
> > +                     (let ((image (create-image (if (not remote-p)
> > +                                                    file
> > +                                                  (with-temp-buffer
> > +
> (insert-file-contents file)
> > +                                                    (string-make-unibyte
> > +
>  (buffer-substring-no-properties
> > +                                                      (point-min)
> (point-max)))))
> >                                                  (and
> (image-type-available-p 'imagemagick)
> >                                                       width 'imagemagick)
> > -                                                nil
> > +                                                remote-p
> >                                                  :width width)))
> >                         (when image
> >                           (let ((ov (make-overlay
>
> FWIW, looks good to me, but I've only (carefully) read the patch: I have
> not actually ran it.
>
> --
> Nick
>
> "There are only two hard problems in computer science: cache
> invalidation, naming things, and off-by-one errors." -Martin Fowler
>
>
>

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

Re: Displaying remote images

[Jack Kamm]

After test-driving this patch for a bit, the main issue I foresee is
that this can potentially hang Emacs if loading a lot of images over a
slow SSH connection. This was also discussed in the previous thread from
2014.

I plan to mitigate this with an option to enable/disable/cache the
remote images. For the cache'ing, visiting the remote files in their own
Emacs buffers seems to work pretty well.

I'm in the process of updating my copyright papers for my current
employer, and will send an updated patch when I've got that sorted out.

--Jack

Re: Displaying remote images

[Jack Kamm]

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

Hi there,

Apologies for the delay on this. I've now got a more complete patch for
displaying remote images inline. Since downloading many remote images
could potentially hang Emacs on a slow connection, I've added an option
to control whether remote images are displayed. I've also added an
option to cache the remote images by visiting them in Emacs buffers.

The default behavior is not to display remote images, but to issue a
message that references the option that controls remote image display.

Best,
Jack


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org.el-Add-inline-remote-image-display.patch --]
[-- Type: text/x-patch, Size: 3769 bytes --]

From 88c37616fc7b910deec34f3013af36ceca8cde9b Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 19 Jan 2020 14:08:01 -0800
Subject: [PATCH] org.el: Add inline remote image display

* lisp/org.el (org-display-inline-images): Add inline remote image
display. Remote image display is controlled by the new option
`org-display-remote-inline-images'.
---
 etc/ORG-NEWS |  6 ++++++
 lisp/org.el  | 53 +++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 67c3ca2ed..d219ff16a 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -35,6 +35,12 @@ value in call to =java=.
 After editing a source block, Org will restore the window layout when
 ~org-src-window-setup~ is set to a value that modifies the layout.
 
+*** Display remote inline images
+
+Added the capability to display remote images inline.  Whether the
+images are actually displayed are controlled by the new option
+~org-display-remote-inline-images~.
+
 ** New functions
 *** ~org-columns-toggle-or-columns-quit~
 =<C-c C-c>= bound to ~org-columns-toggle-or-columns-quit~ replaces the
diff --git a/lisp/org.el b/lisp/org.el
index e011ff61e..383c9ccaf 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16739,6 +16739,53 @@ INCLUDE-LINKED is passed to `org-display-inline-images'."
 ;; For without-x builds.
 (declare-function image-refresh "image" (spec &optional frame))
 
+(defcustom org-display-remote-inline-images 'skip-warn
+  "How to display remote inline images.
+Possible values of this option are:
+
+skip-warn         Don't display, and emit a message about it.
+skip-silent       Don't display, and don't warn about it.
+download-always   Always download and display remote images.
+cache-in-buffer   Display remote images, and open them in separate buffers for
+                  cache'ing.  Silently update the image buffer when a file
+                  change is detected."
+  :type '(choice
+	  (const skip-warn)
+	  (const skip-silent)
+	  (const download-always)
+	  (const cache-in-buffers))
+  :group 'org-appearance)
+
+(defun org-inline-image--buffer-unibyte ()
+  (string-make-unibyte (buffer-substring-no-properties
+			(point-min) (point-max))))
+
+(defun org-inline-image--create (file width)
+  (let* ((remote-p (file-remote-p file))
+	 (file-or-data
+	  (if remote-p
+	      (pcase org-display-remote-inline-images
+		(`download-always (with-temp-buffer (insert-file-contents file)
+						    (org-inline-image--buffer-unibyte)))
+		(`cache-in-buffers (let ((revert-without-query '(".*")))
+				     (with-current-buffer
+					 (find-file-noselect file)
+				       (org-inline-image--buffer-unibyte))))
+		(`skip-warn (message
+			     (concat "Set `org-display-remote-inline-images'"
+				     " to display remote images."))
+			    nil)
+		(`skip-silent nil)
+		(_ (message (concat "Invalid value of "
+				    "`org-display-remote-inline-images'"))
+		   nil))
+	    file)))
+    (when file-or-data
+      (create-image file-or-data
+		    (and (image-type-available-p 'imagemagick)
+			 width 'imagemagick)
+		    remote-p :width width))))
+
 (defun org-display-inline-images (&optional include-linked refresh beg end)
   "Display inline images.
 
@@ -16857,11 +16904,7 @@ buffer boundaries with possible narrowing."
 				'org-image-overlay)))
 		      (if (and (car-safe old) refresh)
 			  (image-refresh (overlay-get (cdr old) 'display))
-			(let ((image (create-image file
-						   (and (image-type-available-p 'imagemagick)
-							width 'imagemagick)
-						   nil
-						   :width width)))
+			(let ((image (org-inline-image--create file width)))
 			  (when image
 			    (let ((ov (make-overlay
 				       (org-element-property :begin link)
-- 
2.25.0

Re: Displaying remote images

[Nicolas Goaziou]

Hello,

Jack Kamm <jackkamm@gmail.com> writes:

> Apologies for the delay on this. I've now got a more complete patch for
> displaying remote images inline. Since downloading many remote images
> could potentially hang Emacs on a slow connection, I've added an option
> to control whether remote images are displayed. I've also added an
> option to cache the remote images by visiting them in Emacs buffers.

Thank you.

> The default behavior is not to display remote images, but to issue a
> message that references the option that controls remote image display.

I think displaying a message in this case can be annoying. It means the
default value is not satisfying for anyone.

> +(defcustom org-display-remote-inline-images 'skip-warn
> +  "How to display remote inline images.
> +Possible values of this option are:
> +
> +skip-warn         Don't display, and emit a message about it.
> +skip-silent       Don't display, and don't warn about it.
> +download-always   Always download and display remote images.
> +cache-in-buffer   Display remote images, and open them in separate buffers for
> +                  cache'ing.  Silently update the image buffer when a file
> +                  change is detected."
> +  :type '(choice
> +	  (const skip-warn)
> +	  (const skip-silent)
> +	  (const download-always)
> +	  (const cache-in-buffers))
> +  :group 'org-appearance)

I suggest to drop the `skip-warn' value altogether, and use
`skip-silent', renamed as `skip', as the default value.

It also needs a :package-version and a :safe keyword.

> +(defun org-inline-image--buffer-unibyte ()
> +  (string-make-unibyte (buffer-substring-no-properties
> +			(point-min) (point-max))))

I'm surprised such a function is necessary. In any case, it should be
named `org--inline-image-buffer-unibyte'.

> +(defun org-inline-image--create (file width)

It should be named `org--inline-image-create' or
`org--create-inline-image'.

> +  (let* ((remote-p (file-remote-p file))
> +	 (file-or-data
> +	  (if remote-p

I suggest to move the `if' within the `pcase' with an initial `guard'.

> +	      (pcase org-display-remote-inline-images
> +		(`download-always (with-temp-buffer (insert-file-contents file)
> +						    (org-inline-image--buffer-unibyte)))

Wouldn't `insert-file-contents-literally' fit the bill instead?

> +		(`cache-in-buffers (let ((revert-without-query '(".*")))
> +				     (with-current-buffer
> +					 (find-file-noselect file)
> +				       (org-inline-image--buffer-unibyte))))

Wouldn't the RAW argument from `find-file-noselect' prevent
`org-inline-image--buffer-unibyte' from being used?

> +		(`skip-warn (message
> +			     (concat "Set `org-display-remote-inline-images'"
> +				     " to display remote images."))

Use continuation delimiter instead.

> +			    nil)
> +		(`skip-silent nil)
> +		(_ (message (concat "Invalid value of "
> +				    "`org-display-remote-inline-images'"))

Ditto.

Regards,

-- 
Nicolas Goaziou

Re: Displaying remote images

[stardiviner]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Jack Kamm <jackkamm@gmail.com> writes:
>
>> Apologies for the delay on this. I've now got a more complete patch for
>> displaying remote images inline. Since downloading many remote images
>> could potentially hang Emacs on a slow connection, I've added an option
>> to control whether remote images are displayed. I've also added an
>> option to cache the remote images by visiting them in Emacs buffers.
>
> Thank you.
>
>> The default behavior is not to display remote images, but to issue a
>> message that references the option that controls remote image display.
>
> I think displaying a message in this case can be annoying. It means the
> default value is not satisfying for anyone.
>
>> +(defcustom org-display-remote-inline-images 'skip-warn
>> +  "How to display remote inline images.
>> +Possible values of this option are:
>> +

I agree.

-- 
[ stardiviner ]
       I try to make every word tell the meaning what I want to express.

       Blog: https://stardiviner.github.io/
       IRC(freenode): stardiviner, Matrix: stardiviner
       GPG: F09F650D7D674819892591401B5DF1C95AE89AC3
      

Re: Displaying remote images

[Jack Kamm]

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

Hi Nicolas --

Thank you for reviewing my patch. I'm attaching an updated patch in
response to your review.

> I think displaying a message in this case can be annoying. It means the
> default value is not satisfying for anyone.
> ...
> I suggest to drop the `skip-warn' value altogether, and use
> `skip-silent', renamed as `skip', as the default value.
>
> It also needs a :package-version and a :safe keyword.

Done.

>> +(defun org-inline-image--buffer-unibyte ()
>> +  (string-make-unibyte (buffer-substring-no-properties
>> +			(point-min) (point-max))))
>
> I'm surprised such a function is necessary.

I originally based that function off the way image-mode.el handles
remote files and other raw data:

https://github.com/emacs-mirror/emacs/blob/7497ee44b471f69ce59d131a6dece261e871534f/lisp/image-mode.el#L753

I re-tested whether the call to string-make-unibyte was actually
necessary, using JPG, PNG, and SVG images as test cases. I found it
wasn't necessary for cache-buffer (which visits the file in a persistent
buffer), but it was necessary for the download-always option (which uses
a temp buffer). Otherwise, the download-always option would display JPG
and PNG images as empty boxes (SVG images worked fine though).

One downside of string-make-unibyte, is that "make compile" complains
its a deprecated function. So I switched to using set-buffer-multibyte
instead, which solved the empty box issue without a deprecation warning.

>> +(defun org-inline-image--create (file width)
>
> It should be named `org--inline-image-create' or
> `org--create-inline-image'.

Done.

>> +  (let* ((remote-p (file-remote-p file))
>> +	 (file-or-data
>> +	  (if remote-p
>
> I suggest to move the `if' within the `pcase' with an initial `guard'.

Done.

>
>> +	      (pcase org-display-remote-inline-images
>> +		(`download-always (with-temp-buffer (insert-file-contents file)
>> +						    (org-inline-image--buffer-unibyte)))
>
> Wouldn't `insert-file-contents-literally' fit the bill instead?

Done, but it still required converting the temporary buffer to unibyte
as noted above.

>> +		(`cache-in-buffers (let ((revert-without-query '(".*")))
>> +				     (with-current-buffer
>> +					 (find-file-noselect file)
>> +				       (org-inline-image--buffer-unibyte))))
>
> Wouldn't the RAW argument from `find-file-noselect' prevent
> `org-inline-image--buffer-unibyte' from being used?

Unibyte conversion wasn't required here in my tests, regardless of the
RAWFILE argument, so I've removed it.

For now I've opted not to use the RAWFILE argument. My thinking is that
the user might want to view the image in its own buffer later, and if
the RAWFILE argument is set, then they'd either see the raw file, or get
a message asking to revisit the file normally.

>> +		(`skip-warn (message
>> +			     (concat "Set `org-display-remote-inline-images'"
>> +				     " to display remote images."))
>
> Use continuation delimiter instead.

Fixed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org.el-Add-inline-remote-image-display.patch --]
[-- Type: text/x-patch, Size: 3420 bytes --]

From e0f61b9043de88161752f34d449ec173e7540202 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 19 Jan 2020 14:08:01 -0800
Subject: [PATCH] org.el: Add inline remote image display

* lisp/org.el (org-display-inline-images): Add inline remote image
display. Remote image display is controlled by the new option
`org-display-remote-inline-images'.
---
 etc/ORG-NEWS |  6 ++++++
 lisp/org.el  | 45 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 90e612529..e2c53d043 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -35,6 +35,12 @@ value in call to =java=.
 After editing a source block, Org will restore the window layout when
 ~org-src-window-setup~ is set to a value that modifies the layout.
 
+*** Display remote inline images
+
+Added the capability to display remote images inline.  Whether the
+images are actually displayed are controlled by the new option
+~org-display-remote-inline-images~.
+
 ** New functions
 *** ~org-columns-toggle-or-columns-quit~
 =<C-c C-c>= bound to ~org-columns-toggle-or-columns-quit~ replaces the
diff --git a/lisp/org.el b/lisp/org.el
index e011ff61e..d6a591a6a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16739,6 +16739,45 @@ INCLUDE-LINKED is passed to `org-display-inline-images'."
 ;; For without-x builds.
 (declare-function image-refresh "image" (spec &optional frame))
 
+(defcustom org-display-remote-inline-images 'skip
+  "How to display remote inline images.
+Possible values of this option are:
+
+skip              Don't display remote images.
+download-always   Always download and display remote images.
+cache-buffer      Display remote images, and open them in separate buffers for
+                  cacheing.  Silently update the image buffer when a file
+                  change is detected."
+  :group 'org-appearance
+  :package-version '(Org . "9.4")
+  :type '(choice
+	  (const skip)
+	  (const download-always)
+	  (const cache-buffer))
+  :safe #'symbolp)
+
+(defun org--create-inline-image (file width)
+  (let* ((remote-p (file-remote-p file))
+	 (file-or-data
+	  (pcase org-display-remote-inline-images
+	    ((guard (not remote-p)) file)
+	    (`download-always (with-temp-buffer
+				(set-buffer-multibyte nil)
+				(insert-file-contents-literally file)
+				(buffer-string)))
+	    (`cache-buffer (let ((revert-without-query '(".*")))
+			     (with-current-buffer
+				 (find-file-noselect file)
+			       (buffer-string))))
+	    (`skip nil)
+	    (_ (message "Invalid value of `org-display-remote-inline-images'")
+	       nil))))
+    (when file-or-data
+      (create-image file-or-data
+		    (and (image-type-available-p 'imagemagick)
+			 width 'imagemagick)
+		    remote-p :width width))))
+
 (defun org-display-inline-images (&optional include-linked refresh beg end)
   "Display inline images.
 
@@ -16857,11 +16896,7 @@ buffer boundaries with possible narrowing."
 				'org-image-overlay)))
 		      (if (and (car-safe old) refresh)
 			  (image-refresh (overlay-get (cdr old) 'display))
-			(let ((image (create-image file
-						   (and (image-type-available-p 'imagemagick)
-							width 'imagemagick)
-						   nil
-						   :width width)))
+			(let ((image (org--create-inline-image file width)))
 			  (when image
 			    (let ((ov (make-overlay
 				       (org-element-property :begin link)
-- 
2.25.0

Re: Displaying remote images

[Nicolas Goaziou]

Hello,

Jack Kamm <jackkamm@gmail.com> writes:

> Thank you for reviewing my patch. I'm attaching an updated patch in
> response to your review.

Applied. Thank you. 

I also simplified the allowed symbols for the new variable a bit, and
added a docstring for the internal function.

Regards,

-- 
Nicolas Goaziou