[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Displaying remote images
From: |
Nicolas Goaziou |
Subject: |
Re: Displaying remote images |
Date: |
Tue, 21 Jan 2020 17:39:45 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Hello,
Jack Kamm <address@hidden> 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