From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kit-Yan Choi Subject: Re: [Patch] org-display-inline-images: Add support for remote images Date: Tue, 25 Nov 2014 10:45:50 -0500 Message-ID: References: <87vbm3of2o.fsf@selenimh.mobile.lan> <87bnnv1e95.fsf@gmx.de> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a11c13e4a3b3e150508b0ce1c Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtIJN-0005ri-Hh for emacs-orgmode@gnu.org; Tue, 25 Nov 2014 10:46:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtIJL-0006c4-Ia for emacs-orgmode@gnu.org; Tue, 25 Nov 2014 10:45:53 -0500 Received: from mail-qc0-x230.google.com ([2607:f8b0:400d:c01::230]:50135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtIJL-0006c0-Ag for emacs-orgmode@gnu.org; Tue, 25 Nov 2014 10:45:51 -0500 Received: by mail-qc0-f176.google.com with SMTP id i17so608954qcy.35 for ; Tue, 25 Nov 2014 07:45:50 -0800 (PST) In-Reply-To: <87bnnv1e95.fsf@gmx.de> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Michael Albinus Cc: emacs-orgmode@gnu.org --001a11c13e4a3b3e150508b0ce1c Content-Type: text/plain; charset=UTF-8 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 wrote: > Kit-Yan Choi 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. > --001a11c13e4a3b3e150508b0ce1c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Michael,

Thanks for the suggestion.=C2= =A0 Indeed `file-local-copy' would have made the code cleaner.=C2=A0 Ye= t `file-local-copy' generates a new filename each time it's run.=C2= =A0 But I wanted to allow the code to check whether the remote image has al= ready been fetched before and so skip unnecessary file transfer, which take= s time (think of it like "rsync -a").=C2=A0 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 &l= t;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.

--001a11c13e4a3b3e150508b0ce1c--