emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Jack Kamm <jackkamm@gmail.com>
Cc: Nick Dokos <ndokos@gmail.com>, emacs-orgmode@gnu.org
Subject: Re: Displaying remote images
Date: Tue, 21 Jan 2020 17:39:45 +0100	[thread overview]
Message-ID: <87zhegpxv2.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <87blqz5bwp.fsf@gmail.com> (Jack Kamm's message of "Sun, 19 Jan 2020 14:17:26 -0800")

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

  reply	other threads:[~2020-01-21 16:39 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-28 17:09 Displaying remote images Jack Kamm
2019-11-28 21:50 ` briangpowell .
2019-11-29  1:39   ` Jack Kamm
2019-11-29  2:00 ` Jack Kamm
2019-11-29  5:36   ` briangpowell .
2019-12-02 20:27   ` Nick Dokos
2019-12-02 22:39     ` briangpowell .
2019-12-07 14:41     ` Jack Kamm
2020-01-19 22:17       ` Jack Kamm
2020-01-21 16:39         ` Nicolas Goaziou [this message]
2020-01-22 15:31           ` stardiviner
2020-01-25  0:28           ` Jack Kamm
2020-02-01 10:51             ` Nicolas Goaziou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zhegpxv2.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=emacs-orgmode@gnu.org \
    --cc=jackkamm@gmail.com \
    --cc=ndokos@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).