Re: [O] [Patch] org-display-inline-images: Add support for remote images

[Kit-Yan Choi]

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 <address@hidden> 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 <address@hidden> wrote:

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

0001-org.el.diff
Description: Text document