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

[Ihor Radchenko <yantar92@posteo.net>]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

>>> +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.
>
> I moved the `refresh' clause out of the display-graphic-p check, but I
> think it's needed.  Otherwise this line will run once per previewed
> image instead of once per call to `org-link-preview':
>
> (when (fboundp 'clear-image-cache) (clear-image-cache))
>
> clearing newly placed previews (within the same run) from the cache.

Sorry, I was not clear. I only referred to `display-graphic-p' check. I
did not mean `clear-image-cache'.

> That said, I have a question about `clear-image-cache' here.  Why does
> Org clear the Emacs frame's entire image cache every time
> `org-link-preview-region' (or the previous `org-display-inline-images')
> is called?  This seems overreaching.  There are many situations even
> within Org mode where we want to avoid the extra file access that
> clearing the image cache will cause, like in a buffer with hundreds of
> LaTeX previews.  Images in unrelated buffers/major-modes are also
> affected.

Here is the original code:

(when refresh
      (org-remove-inline-images beg end)
      (when (fboundp 'clear-image-cache) (clear-image-cache)))

Image cache is cleared _only_ with REFRESH argument.
I think that makes sense, right?

>>> +      (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?
>
> They seemed out of place in ol.el, so my plan was the following:
> 1. This patch: Implement org-link-preview.
> 2. Another patch: Move image creation/manipulation/geometry functions
> from org into an org-image (or org-image-utils) library.  Require it in
> ol.
>
> However, I can move them wherever you want, let me know.

I'd rather see them moved.
You can later send another patch moving them into the new utils library,
if you want.

I am asking because you may or may not have time to create that followup
patch. So, better have things sorted out at every step rather than
relying upon followups that may or may not happen (e.g. for reasons you
cannot control)

>>> +        (when image            ; Add image to overlay
>>> ...
>>> +	  (overlay-put ov 'org-image-overlay t)
>>
>> This is redundant, isn't it?
>
> I don't know how create-image can fail.  Suppose a .jpg file exists but
> is corrupted or does not have image/jpeg data.  Then create-image
> returns nil, as does `org--create-inline-image'.  This check guards
> against trying to preview an "image" that's nil.

Let me elaborate.

In `org-link-preview-region', you set org-image-overlay property:

			     +            (overlay-put ov 'org-image-overlay t)
			     +            (overlay-put ov 'modification-hooks
			     +                         (list 'org-link-preview--remove-overlay))
			     +            ;; call preview function for link type
			     +            (if (funcall preview-func ov path link)
			     +              (when (overlay-buffer ov)

Then, you do it yet again in `org-link-preview-file', when the property
is already there - this part is redundant.

I do not see how `org--create-inline-image' affects the above.

>>> +	  (when (boundp 'image-map)
>>
>> What if not bound? Why not simply (require 'image)?
>
> Just unmodified old code.  I've require'd image in ol now, let me know
> if I should do it differently.

Let's not require it on top level. Maybe better do it within the preview
function.

>> 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.
>
> I'm not sure how to solve this problem.  Here's what I'm picturing --
> note that this doesn't actually avoid the process sentinel pile-up:
>
> The preview provider should return its callback function to Org so Org
> can run it at its convenience.
>
> Org runs (funcall #'preview-func ov path link)
>
> preview-func returns either
> 1. t, if preview is synchronous and successful,
> 2. nil, if preview is synchronous but failed,
> 3. A callback-func, if preview is asynchronous.  In this case the
> preview-func starts the network request or async job.
>
> Org adds callback-func to the queue of callback-funcs for this run, and
> runs them spaced out on a timer or something.  Preview success depends
> on whether the callback-func returns t or nil.
>
> Implementing something like this should be easy with org-async, included
> in the LaTeX preview patchset.
>
> But the process sentinels will run independent of Org's callback queue.
> The callbacks only place the resulting image (or other metadata) on the
> overlays.  The bottleneck here is not the display engine, so this
> doesn't solve the problem.
>
> Can you give me some more details of what you have in mind?

I mostly meant calling preview-func asynchronously, while idle, spaced
out, spending not longer than a fraction of second to call several
preview-funcs.
Spacing might then be controlled by the users.

We might go further, and let the preview functions return a
process. Then, we may explicitly control running sentinels just for that
process via `accept-process-output'. But I am not sure if we need to go
that far.

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