New patch version attached. Not all your points were addressed, please check my comments below. >> Subject: [PATCH] org-link: Move inline image display to org-link > > The patch does a lot more than merely "moving" things around. Please, > name it accordingly. Maybe something like > > New customizable preview API for arbitrary link types Renamed commit. >> +(declare-function org-attach-expand "org-attach" (file)) > > Some of these declares are now redundant. Only one declare was redundant (org-attach-expand), fixed. >> + ;; C-u argument: clear image at point or in entry >> + ((equal arg '(4)) >> + (if (get-char-property (point) 'org-image-overlay) >> + ;; clear link preview at point >> + (when-let ((context (org-element-context)) >> + ((org-element-type-p context 'link))) > > It will be more reliable to clear across overlay boundaries rather than > assuming that the overlay covers exactly one link. Fixed. >> +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. 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. >> + (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. >> + (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. >> + (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. > ... and ORG-NEWS entry. > > You also need to fixup the manual where it talks about image previews. Yes, I will do both at the end, after the changes are final and before merging. >> +`:preview' >> + >> + Function to run to generate an in-buffer preview for the link. It >> + must accept three arguments: >> + - an overlay placed from the start to the end of the link. >> + - the link path, as a string. >> + - the link element >> + This function must return a non-nil value to indicate success. > > 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? Karthik