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

[Karthik Chikmagalur]

v2 of the patch is attached.  I incorporated most of your suggested
changes including in the commit message, except for a couple that I talk
about below.

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.

>> 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?
>
> What I meant is that org-attach will define a custom
> `org-attach-preview-file' function that will compute the filename, and
> then internally call `org-link-preview-file'. Then, attachment: link
> will have :preview set to `org-attach-preview-file'. This way, you will
> not need to require org-attach from ol and ol from org-attach
> simultaneously. (no cyclic dependencies, please)

Done.

> Then, the commit message is not accurate.

Fixed.

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

I decided not to do this since the logic in
org-link-preview--remove-overlay is different: it checks for the
existence of overlays in the maintained list org-link-preview-overlays
instead of checking for overlays in the buffer.

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

Fixed.

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

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

No changes here.  When the async callback runs, the state of the buffer
can have changed.  There's nothing Org can do about that.  The callback
has access to the overlay, so it will have to use a construct like

(and (overlay-buffer ov) (add-preview-to link))

If the preview fails, there will be an undecorated overlay hanging
around, I'm not sure what to do about it.  The async function can run
`(delete-overlay ov)', but that breaks the abstraction boundary we're
trying to create.  (It will also have to remove the overlay from
`org-link-preview-overlays'.)

It can't run `org-link-preview-clear' on the link, since the link
element's bounds might be invalid.  I'm assuming that the :begin and
:end property of the link are integers and not markers.

Karthik

0001-org-link-Move-inline-image-display-to-org-link.patch
Description: Text Data