From: Kit-Yan Choi <kit@kychoi.org>
To: Kit-Yan Choi <kit@kychoi.org>, emacs-orgmode@gnu.org
Subject: Re: [Patch] org-display-inline-images: Add support for remote images
Date: Tue, 25 Nov 2014 10:15:57 -0500 [thread overview]
Message-ID: <CAKHbGXW1ChE7q8iyKrR6PAQsDDu52t=udMe4Jsyn0vB00=4+yw@mail.gmail.com> (raw)
In-Reply-To: <87vbm3of2o.fsf@selenimh.mobile.lan>
[-- Attachment #1.1: Type: text/plain, Size: 5684 bytes --]
Thank you for your comments! They are very helpful.
> 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.
I personally needed this functionality. I have tried to reduce the amount
of time spent on fetching the images by checking whether the images have
been fetched before and whether the remote files are newer. Yes these
communications take time as it should be expected if one opens an org file
remotely (therefore connection should have been made) or when one specifies
a remote image as path and wants to display it inline.
Perhaps I could add an option flag or ask a question before fetching for
user to decide whether to display remote images or not? In case the
behaviour of displaying remote images inline is not desired? One scenario
I can think of is that `org-startup-with-inline-images' is set to true and
the file is sometimes visited remotely.
Any opinion or comment on this, please?
>> - (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?
Because the file path for a remote file, as far as I have tested, have
redundant slashes "/" at the beginning of the path which makes
org-file-remote-p to return nil for a remote path.
`substitute-in-file-name' corrects such path. `substitute-in-file-name' is
also used in `org-open-file'. So I followed suit.
> Are you sure the return value (a boolean, i.e., not necessarily
> a string) should belong to the `concat'?
Good point. I changed the code (see below, and attached).
> (create-image (if (org-file-remote-p file) ...)
> (and width 'imagemagick)
> nil
> :width width)
I agree. Thanks. I made the code cleaner now (see below, and attached).
@@ -19340,7 +19340,7 @@ boundaries."
(not (cdr (org-element-contents parent)))))
(org-string-match-p file-extension-re
(org-element-property :path link)))
- (let ((file (expand-file-name (org-element-property :path link))))
+ (let ((file (substitute-in-file-name (expand-file-name
(org-element-property :path link)))))
(when (file-exists-p file)
(let ((width
;; Apply `org-image-actual-width' specifications.
@@ -19378,10 +19378,24 @@ boundaries."
'org-image-overlay)))
(if (and (car-safe old) refresh)
(image-refresh (overlay-get (cdr old) 'display))
- (let ((image (create-image file
- (and width 'imagemagick)
- nil
- :width width)))
+ (let ((image
+ (create-image (if (org-file-remote-p file)
+ (let* ((tramp-tmpdir (concat
+ (if (featurep 'xemacs)
+ (temp-directory)
+ temporary-file-directory)
+ "/tramp"))
+ (newname (concat
+ tramp-tmpdir
+ (expand-file-name file))))
+ (make-directory tramp-tmpdir t)
+ (if (file-newer-than-file-p file newname)
+ (copy-file file newname t t))
+ newname)
+ file)
+ (and width 'imagemagick)
+ nil
+ :width width)))
(when image
(let* ((link
;; If inline image is the description
On Tue, Nov 25, 2014 at 3:32 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:
> Hello,
>
> Kit-Yan Choi <kit@kychoi.org> 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
>
[-- Attachment #1.2: Type: text/html, Size: 11167 bytes --]
[-- Attachment #2: 0001-org.el.diff --]
[-- Type: text/plain, Size: 1502 bytes --]
diff --git a/lisp/org.el b/lisp/org.el
index 6ab13f4..96b04b6 100755
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19340,7 +19340,7 @@ boundaries."
(not (cdr (org-element-contents parent)))))
(org-string-match-p file-extension-re
(org-element-property :path link)))
- (let ((file (expand-file-name (org-element-property :path link))))
+ (let ((file (substitute-in-file-name (expand-file-name (org-element-property :path link)))))
(when (file-exists-p file)
(let ((width
;; Apply `org-image-actual-width' specifications.
@@ -19378,10 +19378,24 @@ boundaries."
'org-image-overlay)))
(if (and (car-safe old) refresh)
(image-refresh (overlay-get (cdr old) 'display))
- (let ((image (create-image file
- (and width 'imagemagick)
- nil
- :width width)))
+ (let ((image
+ (create-image (if (org-file-remote-p file)
+ (let* ((tramp-tmpdir (concat
+ (if (featurep 'xemacs)
+ (temp-directory)
+ temporary-file-directory)
+ "/tramp"))
+ (newname (concat
+ tramp-tmpdir
+ (expand-file-name file))))
+ (make-directory tramp-tmpdir t)
+ (if (file-newer-than-file-p file newname)
+ (copy-file file newname t t))
+ newname)
+ file)
+ (and width 'imagemagick)
+ nil
+ :width width)))
(when image
(let* ((link
;; If inline image is the description
next prev parent reply other threads:[~2014-11-25 15:16 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-22 4:43 [Patch] org-display-inline-images: Add support for remote images Kit-Yan Choi
2014-11-25 8:32 ` Nicolas Goaziou
2014-11-25 15:15 ` Kit-Yan Choi [this message]
2014-11-25 15:23 ` Rasmus
2014-11-25 15:29 ` Kit-Yan Choi
2014-11-25 15:39 ` Michael Albinus
2014-11-25 15:45 ` Kit-Yan Choi
2014-11-25 17:04 ` Michael Albinus
2014-11-26 17:13 ` Kit-Yan Choi
2014-11-29 10:50 ` Michael Albinus
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='CAKHbGXW1ChE7q8iyKrR6PAQsDDu52t=udMe4Jsyn0vB00=4+yw@mail.gmail.com' \
--to=kit@kychoi.org \
--cc=emacs-orgmode@gnu.org \
/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).