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

[Ihor Radchenko <yantar92@posteo.net>]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

>> Spin out will be easier when for my rebase :)
>
> Please find attached the draft of a patch implementing a general
> link-preview system.

Thanks!

> Not yet handled or final:
>
> - Link abbreviations: I'm using org-link-any-re to find links, and this
>   appears to be handling link abbreviations fine.  So there is no
>   special code to handle link abbreviations.  But I'm not sure.

> - The calling convention for the :preview function is not final.  Right
>   now it is given the overlay, the link type and the link path.  But
>   other link details can be required -- for example, image links need to
>   read the org keywords for the image width and alignment, so I end up
>   calling (org-element-context) again inside the :preview function.

We may simply pass the link object to :preview function.

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

Is there [PATCH 1/2]?

> * lisp/org-keys.el: Bind `C-c C-x C-v' to new command
> `org-link-preview', which has the same prefix arg behaviors as
> `org-latex-preview'.

Didn't we discuss changes to the behavior?

> +`:preview'
> +
> +  Function to run when generating 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 type, as a string.
> +  - the path, as a string.

Maybe we can ask the function to return non-nil when the preview is
going to happen. The current approach with "overlay deleted then no
preview" is not ideal.

> +(defun org-link-preview--get-overlays (&optional beg end)
> +  "Return link preview overlays between BEG and END."
> +  (let* ((beg (or beg (point-min)))
> +         (end (or end (point-max)))
> +         (overlays (overlays-in beg end))
> +         result)
> +    (dolist (ov overlays result)
> +      (when (memq ov org-link-preview-overlays)
> +        (push ov result)))))

I know that this is a copy-paste, but we may utilize 'org-image-overlay
+ `org-find-overlays' here.

> +(defun org-link-preview--remove-overlay (ov after _beg _end &optional _len)
> +  "Remove link-preview overlay OV if a corresponding region is modified.
> +
> +AFTER is true when this function is called post-change."
> +  (when (and ov after)
> +    (setq org-link-preview-overlays (delete ov org-link-preview-overlays))
> +    ;; Clear image from cache to avoid image not updating upon
> +    ;; changing on disk.  See Emacs bug#59902.
> +    (when (overlay-get ov 'org-image-overlay)
> +      (image-flush (overlay-get ov 'display)))
> +    (delete-overlay ov)))

It implies that every :preview function _must_ put an image as 'display
property. If it does not, we will run into errors. But should it?

Also, when if preview is asynchronous, and we run this function when the
image is not yet assigned to the overlay?

> +    (cond
> +     ((not (display-graphic-p))
> +      (message "Your Emacs does not support displaying images!"))

May some third-party previews not require graphic?

> +(defun org-link-preview-clear (&optional beg end)
> +  "Clear link previews in region BEG to END."
> +  (interactive (and (use-region-p) (list (region-beginning) (region-end))))
> +  (let* ((beg (or beg (point-min)))
> +         (end (or end (point-max)))
> +         (overlays (overlays-in beg end)))
> +    (dolist (ov overlays)
> +      (when (memq ov org-link-preview-overlays)
> +        (when-let ((image (overlay-get ov 'display))
> +                   ((imagep image)))
> +          (image-flush image))
> +        (setq org-link-preview-overlays (delq ov org-link-preview-overlays))
> +        (delete-overlay ov)))

In asynchronous previews, some overlays may be deleted already. We
should be careful with this.

> +(defun org-link-preview-file (ov linktype path)
> +  "Display image file PATH in overlay OV.
> +
> +LINKTYPE is the Org link type used to preview PATH, either
> +\"file\" or \"attachment\".
> +
> +Equip each image with the keymap `image-map'.
> +
> +This is intended to be used as the `:preview' link property of
> +file links, see `org-link-parameters'."
> +  (if-let ((file-full
> +            (if (equal "attachment" linktype)
> +		(progn
> +                  (require 'org-attach)
> +		  (ignore-errors (org-attach-expand path)))
> +              (expand-file-name path)))

I'd rather put this part into org-attach, as a separate function that
calls `org-link-preview-file'.

-- 
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>