* [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).