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:29:35 -0500 [thread overview]
Message-ID: <CAKHbGXURH9vm1V3Zk2Su1Z7Ac9s+cpFasdmy-g46ysHMS0_TwQ@mail.gmail.com> (raw)
In-Reply-To: <CAKHbGXW1ChE7q8iyKrR6PAQsDDu52t=udMe4Jsyn0vB00=4+yw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 7532 bytes --]
Ah my apologies. I forgot I had to use `file-name-directory' for creating
the path to the temporary directory (too long ago since I did this). Here
is the corrected version.
--- 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,25 @@ 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"
+ (file-name-directory (expand-file-name file))))
+ (newname (concat
+ tramp-tmpdir
+ (file-name-nondirectory (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 10:15 AM, Kit-Yan Choi <kit@kychoi.org> wrote:
> 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: 15551 bytes --]
[-- Attachment #2: 0001-org.el.diff --]
[-- Type: text/plain, Size: 1586 bytes --]
diff --git a/lisp/org.el b/lisp/org.el
index 6ab13f4..44ca7d6 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,25 @@ 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"
+ (file-name-directory (expand-file-name file))))
+ (newname (concat
+ tramp-tmpdir
+ (file-name-nondirectory (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:29 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
2014-11-25 15:23 ` Rasmus
2014-11-25 15:29 ` Kit-Yan Choi [this message]
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=CAKHbGXURH9vm1V3Zk2Su1Z7Ac9s+cpFasdmy-g46ysHMS0_TwQ@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).