From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jack Kamm Subject: Re: Displaying remote images Date: Fri, 24 Jan 2020 16:28:33 -0800 Message-ID: <871rro1iry.fsf@gmail.com> References: <8736e8sza3.fsf@gmail.com> <87tv6nsaqh.fsf@gmail.com> <871rtmbhia.fsf@alphaville.usersys.redhat.com> <87immsdwpf.fsf@gmail.com> <87blqz5bwp.fsf@gmail.com> <87zhegpxv2.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:51152) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iv9JW-0004Nl-DH for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 19:28:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iv9JU-00056N-SY for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 19:28:38 -0500 Received: from mail-pj1-x1035.google.com ([2607:f8b0:4864:20::1035]:36686) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iv9JU-00053P-Jp for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 19:28:36 -0500 Received: by mail-pj1-x1035.google.com with SMTP id gv17so558990pjb.1 for ; Fri, 24 Jan 2020 16:28:36 -0800 (PST) In-Reply-To: <87zhegpxv2.fsf@nicolasgoaziou.fr> 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-mx.org@gnu.org Sender: "Emacs-orgmode" To: Nicolas Goaziou Cc: Nick Dokos , emacs-orgmode@gnu.org --=-=-= Content-Type: text/plain Hi Nicolas -- Thank you for reviewing my patch. I'm attaching an updated patch in response to your review. > I think displaying a message in this case can be annoying. It means the > default value is not satisfying for anyone. > ... > I suggest to drop the `skip-warn' value altogether, and use > `skip-silent', renamed as `skip', as the default value. > > It also needs a :package-version and a :safe keyword. Done. >> +(defun org-inline-image--buffer-unibyte () >> + (string-make-unibyte (buffer-substring-no-properties >> + (point-min) (point-max)))) > > I'm surprised such a function is necessary. I originally based that function off the way image-mode.el handles remote files and other raw data: https://github.com/emacs-mirror/emacs/blob/7497ee44b471f69ce59d131a6dece261e871534f/lisp/image-mode.el#L753 I re-tested whether the call to string-make-unibyte was actually necessary, using JPG, PNG, and SVG images as test cases. I found it wasn't necessary for cache-buffer (which visits the file in a persistent buffer), but it was necessary for the download-always option (which uses a temp buffer). Otherwise, the download-always option would display JPG and PNG images as empty boxes (SVG images worked fine though). One downside of string-make-unibyte, is that "make compile" complains its a deprecated function. So I switched to using set-buffer-multibyte instead, which solved the empty box issue without a deprecation warning. >> +(defun org-inline-image--create (file width) > > It should be named `org--inline-image-create' or > `org--create-inline-image'. Done. >> + (let* ((remote-p (file-remote-p file)) >> + (file-or-data >> + (if remote-p > > I suggest to move the `if' within the `pcase' with an initial `guard'. Done. > >> + (pcase org-display-remote-inline-images >> + (`download-always (with-temp-buffer (insert-file-contents file) >> + (org-inline-image--buffer-unibyte))) > > Wouldn't `insert-file-contents-literally' fit the bill instead? Done, but it still required converting the temporary buffer to unibyte as noted above. >> + (`cache-in-buffers (let ((revert-without-query '(".*"))) >> + (with-current-buffer >> + (find-file-noselect file) >> + (org-inline-image--buffer-unibyte)))) > > Wouldn't the RAW argument from `find-file-noselect' prevent > `org-inline-image--buffer-unibyte' from being used? Unibyte conversion wasn't required here in my tests, regardless of the RAWFILE argument, so I've removed it. For now I've opted not to use the RAWFILE argument. My thinking is that the user might want to view the image in its own buffer later, and if the RAWFILE argument is set, then they'd either see the raw file, or get a message asking to revisit the file normally. >> + (`skip-warn (message >> + (concat "Set `org-display-remote-inline-images'" >> + " to display remote images.")) > > Use continuation delimiter instead. Fixed. --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-org.el-Add-inline-remote-image-display.patch >From e0f61b9043de88161752f34d449ec173e7540202 Mon Sep 17 00:00:00 2001 From: Jack Kamm Date: Sun, 19 Jan 2020 14:08:01 -0800 Subject: [PATCH] org.el: Add inline remote image display * lisp/org.el (org-display-inline-images): Add inline remote image display. Remote image display is controlled by the new option `org-display-remote-inline-images'. --- etc/ORG-NEWS | 6 ++++++ lisp/org.el | 45 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index 90e612529..e2c53d043 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -35,6 +35,12 @@ value in call to =java=. After editing a source block, Org will restore the window layout when ~org-src-window-setup~ is set to a value that modifies the layout. +*** Display remote inline images + +Added the capability to display remote images inline. Whether the +images are actually displayed are controlled by the new option +~org-display-remote-inline-images~. + ** New functions *** ~org-columns-toggle-or-columns-quit~ == bound to ~org-columns-toggle-or-columns-quit~ replaces the diff --git a/lisp/org.el b/lisp/org.el index e011ff61e..d6a591a6a 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -16739,6 +16739,45 @@ INCLUDE-LINKED is passed to `org-display-inline-images'." ;; For without-x builds. (declare-function image-refresh "image" (spec &optional frame)) +(defcustom org-display-remote-inline-images 'skip + "How to display remote inline images. +Possible values of this option are: + +skip Don't display remote images. +download-always Always download and display remote images. +cache-buffer Display remote images, and open them in separate buffers for + cacheing. Silently update the image buffer when a file + change is detected." + :group 'org-appearance + :package-version '(Org . "9.4") + :type '(choice + (const skip) + (const download-always) + (const cache-buffer)) + :safe #'symbolp) + +(defun org--create-inline-image (file width) + (let* ((remote-p (file-remote-p file)) + (file-or-data + (pcase org-display-remote-inline-images + ((guard (not remote-p)) file) + (`download-always (with-temp-buffer + (set-buffer-multibyte nil) + (insert-file-contents-literally file) + (buffer-string))) + (`cache-buffer (let ((revert-without-query '(".*"))) + (with-current-buffer + (find-file-noselect file) + (buffer-string)))) + (`skip nil) + (_ (message "Invalid value of `org-display-remote-inline-images'") + nil)))) + (when file-or-data + (create-image file-or-data + (and (image-type-available-p 'imagemagick) + width 'imagemagick) + remote-p :width width)))) + (defun org-display-inline-images (&optional include-linked refresh beg end) "Display inline images. @@ -16857,11 +16896,7 @@ buffer boundaries with possible narrowing." 'org-image-overlay))) (if (and (car-safe old) refresh) (image-refresh (overlay-get (cdr old) 'display)) - (let ((image (create-image file - (and (image-type-available-p 'imagemagick) - width 'imagemagick) - nil - :width width))) + (let ((image (org--create-inline-image file width))) (when image (let ((ov (make-overlay (org-element-property :begin link) -- 2.25.0 --=-=-=--