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

[Karthik Chikmagalur]

Latest version attached.

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

Done.

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

Please try it now, it should be fixed.  I now set the
`org-image-overlay' when the preview is done/successful, instead of when
it is queued.

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

This quadratic check is still present.

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

It turns out `org-link-preview-overlays' is now required again, since
the overlay property `org-image-overlay' is only placed after the
preview is done, so we can't use it to collect overlays anymore.

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

Thanks.

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

I will check this in the final version, along with the manual/NEWS
updates.

Karthik

0001-org-link-Customizable-preview-API-for-arbitrary-link.patch
Description: Text Data