[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [Patch] org-display-inline-images: Add support for remote images
From: |
Nicolas Goaziou |
Subject: |
Re: [O] [Patch] org-display-inline-images: Add support for remote images |
Date: |
Tue, 25 Nov 2014 09:32:47 +0100 |
Hello,
Kit-Yan Choi <address@hidden> writes:
> I would like to submit a patch to support displaying remote images inline
> in Org-mode. Attached is the formatted patch (or github branch here
> <https://github.com/kitchoi/org-mode/commit/2e600da455c371754f028ddaaed1ae1724cbe6b6>
> .)
Thanks for your patch. However, I wonder if we really want this. Remote
images could be slow to fetch, and it would make buffer unusable.
> Feedbacks are most welcome. Thanks.
Some comments follow.
> - (let ((file (expand-file-name (org-element-property :path link))))
> + (let ((file (substitute-in-file-name (expand-file-name
> (org-element-property :path link)))))
Why is it needed?
> + (let* ((image
> + (let ((newname
> + (if (org-file-remote-p file)
> + (let* ((tramp-tmpdir (concat
> + (if (featurep
> 'xemacs)
> +
> (temp-directory)
> +
> temporary-file-directory)
> + "/tramp"
> +
> (org-file-remote-p file)
^^^^^^^^^^^^^^^^^^^^^^^^
Are you sure the return value (a boolean, i.e., not necessarily
a string) should belong to the `concat'?
> +
> (file-name-directory
> +
> (org-babel-local-file-name file))))
> + (newname (concat
> + tramp-tmpdir
> +
> (file-name-nondirectory file))))
> + (make-directory tramp-tmpdir t)
> + (if
> (tramp-handle-file-newer-than-file-p file newname)
> + (tramp-compat-copy-file file
> newname t t))
> + newname)
> + file)))
> + (create-image newname
> + (and width 'imagemagick)
> + nil
> + :width width))))
IMO, it is clearer to use
(create-image (if (org-file-remote-p file) ...)
(and width 'imagemagick)
nil
:width width)
Regards,
--
Nicolas Goaziou
- [O] [Patch] org-display-inline-images: Add support for remote images, Kit-Yan Choi, 2014/11/21
- Re: [O] [Patch] org-display-inline-images: Add support for remote images,
Nicolas Goaziou <=
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Kit-Yan Choi, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Rasmus, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Kit-Yan Choi, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Michael Albinus, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Kit-Yan Choi, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Michael Albinus, 2014/11/25
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Kit-Yan Choi, 2014/11/26
- Re: [O] [Patch] org-display-inline-images: Add support for remote images, Michael Albinus, 2014/11/29