* [Patch] org-display-inline-images: Add support for remote images
@ 2014-11-22 4:43 Kit-Yan Choi
2014-11-25 8:32 ` Nicolas Goaziou
0 siblings, 1 reply; 10+ messages in thread
From: Kit-Yan Choi @ 2014-11-22 4:43 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1.1: Type: text/plain, Size: 381 bytes --]
Hi,
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>
.)
I have tested the code with "make test". FSF document is signed (waiting
for gnu to send it back).
Feedbacks are most welcome. Thanks.
Kit
[-- Attachment #1.2: Type: text/html, Size: 542 bytes --]
[-- Attachment #2: 0001-org.el.diff --]
[-- Type: text/plain, Size: 1694 bytes --]
diff --git a/lisp/org.el b/lisp/org.el
index 1f33d2a..aaa5c9b 100755
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19338,7 +19338,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.
@@ -19376,10 +19376,29 @@ 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
+ (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)
+ (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))))
(when image
(let* ((link
;; If inline image is the description
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
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
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Goaziou @ 2014-11-25 8:32 UTC (permalink / raw)
To: Kit-Yan Choi; +Cc: emacs-orgmode
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
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
0 siblings, 2 replies; 10+ messages in thread
From: Kit-Yan Choi @ 2014-11-25 15:15 UTC (permalink / raw)
To: Kit-Yan Choi, emacs-orgmode
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 15:15 ` Kit-Yan Choi
@ 2014-11-25 15:23 ` Rasmus
2014-11-25 15:29 ` Kit-Yan Choi
1 sibling, 0 replies; 10+ messages in thread
From: Rasmus @ 2014-11-25 15:23 UTC (permalink / raw)
To: emacs-orgmode
Kit-Yan Choi <kit@kychoi.org> writes:
>> 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?
I recently worked with remote pictures for html-slides. The slides were
stored on my github powered website, and the pictures were hosted on my
owncloud. So I definitely see the merits of Kit-Yan's proposal.
Also, remote files will work in HTML, but not in latex or odt (I think),
so local caching could maybe also be applied, optionally, when exporting
documents.
org-startup-with-inline-images should have an equivalent
file-variable that takes priority, and probably
org-startup-with-inline-images should default to nil.
The above is of course "IMO".
—Rasmus
--
Got mashed potatoes. Ain't got no T-Bone. No T-Bone
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 15:15 ` Kit-Yan Choi
2014-11-25 15:23 ` Rasmus
@ 2014-11-25 15:29 ` Kit-Yan Choi
2014-11-25 15:39 ` Michael Albinus
1 sibling, 1 reply; 10+ messages in thread
From: Kit-Yan Choi @ 2014-11-25 15:29 UTC (permalink / raw)
To: Kit-Yan Choi, emacs-orgmode
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 15:29 ` Kit-Yan Choi
@ 2014-11-25 15:39 ` Michael Albinus
2014-11-25 15:45 ` Kit-Yan Choi
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2014-11-25 15:39 UTC (permalink / raw)
To: Kit-Yan Choi; +Cc: emacs-orgmode
Kit-Yan Choi <kit@kychoi.org> writes:
> 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
This code looks much to complicate to me. Wouldn't a simple
file-local-copy suffice? You don't need to care whether the file is
remote (let Tramp do the job) or local (there won't be a copy).
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 15:39 ` Michael Albinus
@ 2014-11-25 15:45 ` Kit-Yan Choi
2014-11-25 17:04 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Kit-Yan Choi @ 2014-11-25 15:45 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]
Michael,
Thanks for the suggestion. Indeed `file-local-copy' would have made the
code cleaner. Yet `file-local-copy' generates a new filename each time
it's run. But I wanted to allow the code to check whether the remote image
has already been fetched before and so skip unnecessary file transfer,
which takes time (think of it like "rsync -a"). Therefore I wanted to
preserve the remote path under a temporary directory.
Best,
Kit
On Tue, Nov 25, 2014 at 10:39 AM, Michael Albinus <michael.albinus@gmx.de>
wrote:
> Kit-Yan Choi <kit@kychoi.org> writes:
>
> > 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
>
> This code looks much to complicate to me. Wouldn't a simple
> file-local-copy suffice? You don't need to care whether the file is
> remote (let Tramp do the job) or local (there won't be a copy).
>
> Best regards, Michael.
>
[-- Attachment #2: Type: text/html, Size: 3226 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 15:45 ` Kit-Yan Choi
@ 2014-11-25 17:04 ` Michael Albinus
2014-11-26 17:13 ` Kit-Yan Choi
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2014-11-25 17:04 UTC (permalink / raw)
To: Kit-Yan Choi; +Cc: emacs-orgmode
Kit-Yan Choi <kit@kychoi.org> writes:
> Michael,
Hi Kit,
> Thanks for the suggestion. Indeed `file-local-copy' would have made
> the code cleaner. Yet `file-local-copy' generates a new filename each
> time it's run. But I wanted to allow the code to check whether the
> remote image has already been fetched before and so skip unnecessary
> file transfer, which takes time (think of it like "rsync -a").
> Therefore I wanted to preserve the remote path under a temporary
> directory.
After first retrieval via file-local-copy, you could cache this
information somewhere for later use.
Or you could write a ticket towards Tramp, that file-local-copy shall
remember that a file was downloaded already, and should offer that file
for reuse.
Note: I'm the Tramp maintainer :-)
> Best,
> Kit
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-25 17:04 ` Michael Albinus
@ 2014-11-26 17:13 ` Kit-Yan Choi
2014-11-29 10:50 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Kit-Yan Choi @ 2014-11-26 17:13 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]
Thank you! These are interesting ideas! For both of these suggestions, I
think I will need a look-up table using the remote file paths as keys, the
local copy paths as values.
But is it theoretically possible to have two remote file paths sharing the
same local copy file name? i.e. `make-temp-file' generates the same
temporary file name for two separate calls?
If such a situation occurs, then: (1) the local copy of one remote image
will be overwritten by another (2) it is wrong to try to update the local
copy based on the time stamp of a wrong remote file.
As far as org-mode is concerned, and assuming the current stable version of
tramp, I think there are two options (1) the current approach which
preserves the path of the remote file and therefore avoid unnecessary
downloads; however we forgo the capability of compression that tramp's
file-local-copy offers, (2) use `file-local-copy' but risk overwriting
(albeit unlikely) downloaded local copy; this is possible with or with out
a look-up table for reusing files.
And yes, for those who prefer seeing an empty box instead of downloading
remote images for display, I will add an option for turning this
functionality ON/OFF, default off maybe.
Thanks,
Kit
On Tue, Nov 25, 2014 at 12:04 PM, Michael Albinus <michael.albinus@gmx.de>
wrote:
> Kit-Yan Choi <kit@kychoi.org> writes:
>
> > Michael,
>
> Hi Kit,
>
> > Thanks for the suggestion. Indeed `file-local-copy' would have made
> > the code cleaner. Yet `file-local-copy' generates a new filename each
> > time it's run. But I wanted to allow the code to check whether the
> > remote image has already been fetched before and so skip unnecessary
> > file transfer, which takes time (think of it like "rsync -a").
> > Therefore I wanted to preserve the remote path under a temporary
> > directory.
>
> After first retrieval via file-local-copy, you could cache this
> information somewhere for later use.
>
> Or you could write a ticket towards Tramp, that file-local-copy shall
> remember that a file was downloaded already, and should offer that file
> for reuse.
>
> Note: I'm the Tramp maintainer :-)
>
> > Best,
> > Kit
>
> Best regards, Michael.
>
[-- Attachment #2: Type: text/html, Size: 2830 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] org-display-inline-images: Add support for remote images
2014-11-26 17:13 ` Kit-Yan Choi
@ 2014-11-29 10:50 ` Michael Albinus
0 siblings, 0 replies; 10+ messages in thread
From: Michael Albinus @ 2014-11-29 10:50 UTC (permalink / raw)
To: Kit-Yan Choi; +Cc: emacs-orgmode
Kit-Yan Choi <kit@kychoi.org> writes:
> But is it theoretically possible to have two remote file paths sharing
> the same local copy file name? i.e. `make-temp-file' generates the
> same temporary file name for two separate calls?
No, never ever. After choosing a random file name, make-temp-file checks
whether there exists already such a file. In case of yes, it chosses
another random file name, and so on.
> As far as org-mode is concerned, and assuming the current stable
> version of tramp, I think there are two options (1) the current
> approach which preserves the path of the remote file and therefore
> avoid unnecessary downloads; however we forgo the capability of
> compression that tramp's file-local-copy offers, (2) use
> `file-local-copy' but risk overwriting (albeit unlikely) downloaded
> local copy; this is possible with or with out a look-up table for
> reusing files.
Again, the risk of overwriting something else does not exist.
> Thanks,
> Kit
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-11-29 10:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).