Re: [PATCH v2] Inline image display as part of a new org-link-preview system

[Ihor Radchenko <yantar92@posteo.net>]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> First, here's an updated description of the proposed API:
>
> 1. You can register a previewer with each type of Org link in the
> following way:
>
> (org-link-set-parameters "file"  :preview #'org-link-preview-file)
> (org-link-set-parameters "https" :preview #'my/org-link-preview-web)
>
> 2. This previewer is called by Org with three parameters: an overlay,
> the link path being previewed and the link element:
>
> (funcall #'org-link-preview-file ov path link)
>
>    - To preview the link, the previewer must update the overlay ov as
>      needed, for example by placing an image as its `display' property.
>      It should not modify the buffer contents.
>    - The previewer must return a non-nil value to indicate preview
>      success.  If the preview fails, it should return nil.
>
> That's it.

Sounds reasonable.

> Subject: [PATCH] org-link: Move inline image display to org-link

The patch does a lot more than merely "moving" things around. Please,
name it accordingly. Maybe something like

   New customizable preview API for arbitrary link types

> --- a/lisp/ol.el
> +++ b/lisp/ol.el
> @@ -82,6 +82,13 @@ (declare-function org-src-source-buffer "org-src" ())
>  (declare-function org-src-source-type "org-src" ())
>  (declare-function org-time-stamp-format "org" (&optional long inactive))
>  (declare-function outline-next-heading "outline" ())
> +(declare-function image-flush "image" (spec &optional frame))
> +(declare-function org-entry-end-position "org" ())
> +(declare-function org-element-contents-begin "org-element" (node))
> +(declare-function org-attach-expand "org-attach" (file))
> +(declare-function org-display-inline-image--width "org" (link))
> +(declare-function org-image--align "org" (link))
> +(declare-function org--create-inline-image "org" (file width))

Some of these declares are now redundant.
  
> +`:preview'
> +
> +  Function to run to generate an in-buffer preview for the link.  It
> +  must accept three arguments:
> +  - an overlay placed from the start to the end of the link.
> +  - the link path, as a string.
> +  - the link element
> +  This function must return a non-nil value to indicate success.

I am wondering whether asynchronicity should be implemented on high
level rather than inside individual link preview functions.
We may, for example, first create the necessary overlays and indicate
that they are "processing..." first, and then call the actual preview
functions asynchronously.

If we leave asynchronous handling to individual previews, they will have
no way to handle queuing large number of link previews, and we may arrive
at the common situation with network processes when they schedule and
finish around the same time, hanging Emacs while it is handling
a bunch of process sentinels.

> +     ;; C-u argument: clear image at point or in entry
> +     ((equal arg '(4))
> +      (if (get-char-property (point) 'org-image-overlay)
> +          ;; clear link preview at point
> +          (when-let ((context (org-element-context))
> +                     ((org-element-type-p context 'link)))

It will be more reliable to clear across overlay boundaries rather than
assuming that the overlay covers exactly one link.

> +BEG and END define the considered part.  They default to the
> +buffer boundaries with possible narrowing."
> +  (interactive "P")
> +  (when (display-graphic-p)

Do we need it here? You check graphics both here and later in the
preview function. We may probably drop the check herein.

> +      (let* ((width (org-display-inline-image--width link))
> +	     (align (org-image--align link))
> +             (image (org--create-inline-image file width)))

I am wondering why you left these functions in org.el. Why not moving
them here?

> +        (when image            ; Add image to overlay
> ...
> +	  (overlay-put ov 'org-image-overlay t)

This is redundant, isn't it?

> +	  (when (boundp 'image-map)

What if not bound? Why not simply (require 'image)?

... and ORG-NEWS entry.

You also need to fixup the manual where it talks about image previews.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>