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

[Ihor Radchenko <yantar92@posteo.net>]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> If you think it's a good idea, I can add pending previews to the queue
> in a LIFO fashion instead, so that if you call `org-link-preview' in two
> different sections before the first one is done previewing, the later
> one is processed first.

Yes, it would make sense.

> If `org-link-preview-clear' is run in a region where there are previews
> pending, these region-specific previews are removed from the queue.

I tried the following

1. Requests 1000 previews in a buffer
2. Move point to section, request previews on a specific image (not yet displayed)
3. Observe "Preview removed" message, which is confusing as no preview
   is visible at that point.

We need to do something about the logic determining whether any previews
are displayed or not. The current check that preview overlays are
present in requested region is no longer accurate as they may still be
queued and not yet visible to the user, creating impression that there
are no previews yet.

> Note that this new code in `org-link-preview-clear' is not actually
> necessary:
>
>    (when (memq ov org-link-preview-overlays)
>      ;; Remove pending preview tasks between BEG and END
>      (when-let ((spec (cl-find ov org-link-preview--queue
>                                :key #'cadr)))
>        (setq org-link-preview--queue
>              (delq spec org-link-preview--queue)))
>
> This is because the overlay placed over the links for which previews are
> pending are removed anyway.  So when the `--process-queue' function gets
> to that preview-pending link, it skips it since the overlay is gone.
> Also, the complexity of the above code is quadratic, so I can remove it
> if you think it's not required.  I included it to be thorough.

What you say make sense, but see the above - we make certain (now
incorrect) assumptions about `org-link-preview-overlays'. The relevant
parts of the code need to be re-checked.

> Also, I think the variable `org-link-preview-overlays' is actually
> unnecessary.  We can just use an overlay property for link-preview
> overlays and use `overlays-in' and `org-find-overlays', and reduce the
> amount of global state a little bit this way.  Let me know if I should
> remove it.

That might be an option. But we cannot remove it. We can only deprecate
it as of now and only remove in following releases.

>> And please fix the compiler warnings.
>
> As of this patch, I don't see any flymake errors with the
> `elisp-flymake-byte-compile' backend.  Is this not sufficient?
> Byte-compiling in my active Emacs session doesn't work as the state is
> "polluted".  I don't know of a convenient way to byte-compile code in a
> sandbox.

Just run make in Org git repo.

> Please note: I need some help with code style, for example
> `org-link--preview-queue' vs `org-link-preview--queue', etc. Let's
> postpone this particular discussion until after the design is final?

I like org-link-preview-- more, but the most important part is to make
things consistent across the function/variable names.

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