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

[Karthik Chikmagalur <karthikchikmagalur@gmail.com>]

>>> Image cache is cleared _only_ with REFRESH argument.
>>> I think that makes sense, right?
>>
>> Yes.  But `org-link-preview-region' is always called with the REFRESH
>> argument set to `t' though.
>
> Sure. What's a problem with that?

Why flush Emacs' entire image cache to preview images in one Org buffer?

This has implications for other Emacs buffers with lots of images, like
Org buffers with LaTeX previews.  I'm not sure about how much it matters
though.

> In theory, we might not need to clear image cache as long as we call
> `image-flush' in `org-link-preview-clear'.

image-flush is called unconditionally in `org-link-preview-file', so
it should be fine to not set REFRESH unconditionally.  I'll keep the
REFRESH argument around but remove the Emacs-wide (clear-image-cache).

>> Moved (require 'image) to inside the `org-link-preview-file'.  I know it
>> doesn't affect performance in any reasonable way, but I'm usually
>> hesitant to do this in functions that run inside loops.
>
> There are pros and cons.
> Ideally, we should have these previews in a separate library, so that
> loading Org mode does not need to require so much staff. Loading time is
> not great already.
>
> My hesitance about top-level require is that ol.el is loaded by default
> and that the preview functionality may or may not be used by the
> users. So, loading image.el ought to be optional.

Okay.

>> ...
>> +      ;; Run preview asynchronously in batches:
>> +      ;; preview-queue is a list of preview-batch, which is a list of preview-spec
>> +      (when (car preview-queue)
>> +        (dolist (preview-batch (nreverse preview-queue))
>> +          (run-with-idle-timer
>> +           org-link-preview-delay nil
>
> It means that you are scheduling every single preview batch to fire at
> the same time. I think that the timers here should run sequentially -
> (1) fire the first batch without delay; (2) wait org-link-preview-delay
> and fire the next batch; (3) ...

You're right, I'll redo this.  Scheduling many idle timers at once is
tricky, the manual even warns against making idle timers setting up
the next one with the same idle time:

--8<---------------cut here---------------start------------->8---
Similarly, do not write an idle timer function that sets up another idle
timer (including the same idle timer) with SECS argument less than or
equal to the current idleness time.  Such a timer will run almost
immediately, and continue running again and again, instead of waiting
for the next time Emacs becomes idle.  The correct approach is to
reschedule with an appropriate increment of the current value of the
idleness time, as described below.
--8<---------------cut here---------------end--------------->8---

So I'll have to do it carefully.

>> +           (lambda (previews)
>> +             ;; (message "queue: %d" preview-queue-size)
>> +             (cl-decf preview-queue-size)
>> +             (dolist (preview-spec (nreverse previews)) ;spec is (preview-func overlay path link)
>> +               (when-let* ((ov (cadr preview-spec))
>> +                           (buf (overlay-buffer ov)))
>
> ov or its buffer may not exit at the time the timer is fired.
> Because, for example, the buffer is killed, or because user edited the
> underlying link before it got displayed.

This is handled by the (when-let* ((buf (overlay-buffer ov)))) part.
(overlay-buffer ov)  is nil if the overlay is gone, or if the buffer is
gone.

Karthik