Re: Displaying remote images

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

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