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

[Karthik Chikmagalur <karthikchikmagalur@gmail.com>]

Thanks Ihor.  Before I send the next iteration of the patch, I'd like
your input on some decisions marked with "Q:" below.

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

If we pass the link object instead of the overlay, it will be the
preview function's job to create overlays as needed.  This overlay
should have special properties (like `org-image-overlay') for previewing
to work correctly.  

Q: Is this okay? Or did you mean we can pass both the link and the
overlay, like this:

(funcall preview-func ov link)

and let the previewer figure out the link type and path details?

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

There is no [PATCH 1/2], sorry about the naming.  What I sent is the
complete patch.

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

Yes, those changes have been implemented, please see the docstring for
org-link-preview.  The behavior is identical to that of
org-latex-preview, but in addition the 1 and the 11 numeric prefix args
are handled specially.

>> +`: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.

Good idea.

>> +(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.

Noted.

>> +(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?

This was an oversight, I meant to add

(when-let ((disp (overlay-get ov 'display))
           ((imagep disp)))
  (image-flush disp))

Will fix.

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

Handled by the above, plus the fact that we delete the overlay.  It's up
to the async callback to handle the case where the overlay no longer exists.

>> +    (cond
>> +     ((not (display-graphic-p))
>> +      (message "Your Emacs does not support displaying images!"))
>
> May some third-party previews not require graphic?

Good point, will handle.

>> +(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.

Noted.

>> +(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'.

Q: I don't follow.  Right now `org-link-preview-file' is the :preview
org-link-parameter of file links and attachments.  Could you explain how
this indirection should work instead?

Karthik