Re: Displaying remote images

[Jack Kamm <jackkamm@gmail.com>]

[-- 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