Re: Displaying remote images

[Jack Kamm]

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.

0001-org.el-Add-inline-remote-image-display.patch
Description: Text Data