emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Nicolas Goaziou
Subject: Re: [O] [Patch] org-display-inline-images: Add support for remote images
Date: Tue, 25 Nov 2014 09:32:47 +0100

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]