> The next batch of comments on org-latex-preview.el (part 4, final)
>
>> (if (and (buffer-live-p (get-buffer org-latex-preview--latex-log))
>> (get-buffer-process org-latex-preview--latex-log))
>> (let ((buf (generate-new-buffer org-latex-preview--latex-log
>> t)))
>> (prog1 buf
>> (buffer-disable-undo buf)
>> (plist-put extended-info :proc-buffers (list buf))))
>> (with-current-buffer ;Else reuse the standard process
>> buffer
>> (get-buffer-create org-latex-preview--latex-log t)
>> (setq buffer-undo-list t)
>> (erase-buffer)
>> (current-buffer)))
>
> This looks strange.
> Why do you need to maintain one org-latex-preview--latex-log buffer all
> the time? I get it that you reference it in the warnings, but does it
> mean that warnings will point to wrong buffers when the error happens in
> alternative buffer created by (generate-new-buffer
> org-latex-preview--latex-log t)?
Yes, this is a bug.
The reason is that it is good for the user to see
"LaTeX compilation for preview failed (error code %d). Please see %s for
details"
where %s is *Org LaTeX Preview Output*
instead of *Org LaTeX Preview Output some-long-hash-123*
or *Org LaTeX Preview Output <N>*
because they can reliably always check the *Org LaTeX Preview Output*
buffer, create commands to switch to it, etc. I have a command to
switch to this buffer when the preview fails. Having a different name
each time makes it difficult to learn the pattern or automate around it.
The times when (generate-new-buffer org-latex-preview--latex-log t) will
be used by the process are actually very rare. You need to have
simultaneous preview compilations going on, which is actually difficult
to do in interactive use. It happens often if you are writing elisp to
do LaTeX preview compilations in several buffers.
So the (generate-new-buffer org-latex-preview--latex-log t) is there
mainly as a fallback.
I have now fixed the bug where the wrong log buffer is reported.
>> ;; Re-generate the remaining fragments.
>> (org-latex-preview--create-image-async
>> (plist-get extended-info :processor)
>> (cdr fragments)
>> :latex-preamble (plist-get extended-info :latex-header)
>> :place-preview-p (plist-get extended-info :place-preview-p))
>
> Is it normal here that the original :latex-processor and
> :appearance-options are not passed?
No, as far as I can tell it's a bug. We just haven't been bitten by it
yet.
Fixed.
>> (unless (plist-get extended-info :fontsize) ...
>
> I feel like all these :errors/:fontsize/whatnot should be documented
> somewhere in a docstring or a comment in the file. They are hard to
> reason about otherwise, unless you are familiar with the code.
I've added a block comment in org-latex-preview--create-image-async
below the comment that describes the async process tree. It lists all
the info plist keys and their purpose.
>> (when (if (and org-latex-preview-process-precompile
>> (re-search-forward "^PRELOADED FILES:" nil t))
>> (re-search-forward "^ *hyperref\\.sty" nil t)
>> (re-search-forward "^(.*hyperref/hyperref\\.sty" nil t))
>
> The filter will be ran in the process buffer, which means that we will
> be looked at global value of org-latex-preview-process-precompile.
> If org-latex-process-precompile have been disabled locally, the filter
> will still see that global value. This is potential source of bugs I
> guess.
Fixed.
>> (concurrentp (eq (plist-get extended-info :processor) 'dvipng))
>
> With these kinds of assumptions, users changing dvipng entry in
> process-alist will run into problems. Maybe we can protect some parts of
> the processor making them constants?
No change should be necessary. The dvipng and latex procseses and
filters run simultaneously when dvipng is called with the --follow
argument, and the information for a LaTeX fragment (image, geometry etc)
might be populated from the two filters in either order. This line just
handles that case. If they are not running concurrently, only the
dvipng filter (which runs later) populates fragments-to-show.
>> (if (>= org-latex-preview--dvisvgm3-minor-version 1)
>> (mapc #'org-latex-preview--await-fragment-existance
>> fragments-to-show)
>> (mapc #'org-latex-preview--svg-make-fg-currentColor
>> fragments-to-show))
>
> This looks weird.
> `org-latex-preview--await-fragment-existance' simply watches for file,
> while `org-latex-preview--svg-make-fg-currentColor' calls
> `org-latex-preview--await-fragment-existance' and does something else.
> It would be cleaner to call org-latex-preview--await-fragment-existence
> outside if and then call org-latex-preview--svg-make-fg-currentColor'
> only when version is old.
It looks like something went wrong in a historical rebase,
org-latex-preview--svg-make-fg-currentColor shouldn't be calling
org-latex-preview--await-fragment-existence since it implements that
logic itself.
I've removed the call to --await-fragment-existence inside
--svg-make-fg-currentColor.
It is indeed the case that --svg-make-fg-currentColor does what
--await-fragment-existence does and more. I wrote it this way on
purpose. Assuming that make-fg-currentColor doesn't also
do the await-fragment-existence job, the alternative is:
(dolist (svg-fragment fragments-to-show)
(org-latex-preview--await-fragment-existance svg-fragment)
(unless (>= org-latex-preview--dvisvgm3-minor-version 1)
(org-latex-preview--svg-make-fg-currentColor svg-fragment)))
Note that the following looks cleaner but should be avoided, since we
don't want to wait until the existence of _all_ the svg-fragments before
processing their currentColors:
(mapc #'org-latex-preview--await-fragment-existance fragments-to-show)
(unless (>= org-latex-preview--dvisvgm3-minor-version 1)
(mapc #'org-latex-preview--svg-make-fg-currentColor fragments-to-show))
That would be slower with a batch of hundreds of svg-fragments in one
filter function call.
I decided against the dolist approach because I didn't want to run
through the unless conditional (potentially) thousands of times. I
thought that the slight code duplication/non-orthogonal code here is
excusable since all the involved functions and calls are right next to
each other.
No change to the code (so far).
>> (defun org-latex-preview--svg-make-fg-currentColor (svg-fragment)
>> (org-latex-preview--await-fragment-existance svg-fragment)
>> (when path
>> (catch 'svg-exists
>> (dotimes (_ 1000) ; Check for svg existance over 1s.
>> (when (file-exists-p path)
>> (throw 'svg-exists t))
>> (sleep-for 0.001)))
>
> This seemingly repeats what org-latex-preview--await-fragment-existence does.
Removed the call, as mentioned above.
>> (unless ; When the svg is incomplete, wait for it to be completed.
>> (string= (buffer-substring (max 1 (- (point-max) 6)) (point-max))
>> "</svg>")
>> (catch 'svg-complete
>> (dotimes (_ 1000) ; Check for complete svg over 1s.
>> (if (string= (buffer-substring (max 1 (- (point-max) 6))
>> (point-max))
>> "</svg>")
>> (throw 'svg-complete t)
>> (erase-buffer)
>> (sleep-for 0.001)
>> (insert-file-contents path)))
>> (erase-buffer)))
>
> Why not doing all that in org-latex-preview--await-fragment-existence?
Because we don't need to edit the svg when
(>= org-latex-preview--dvisvgm3-minor-version 1)
So we don't need to wait for the file to be fully written, just
verifying the existence of the file at PATH is sufficient.
>> (defun org-latex-preview--place-images (extended-info &optional fragments)
>> If this is an export run, images will only be cached, not placed."
>
> Is org-latex-preview--place-images ever called as export run?
> Or is "export run" flagged by :place-preview-p?
It is always called: org-latex-preview--place-images is to be understood
as placing the image in the cache and optionally in the buffer overlays.
And yes, export runs don't set :place-preview-p.
>> ;; While /in theory/ this check isn't needed, sometimes the
>> ;; org-persist cache can be modified outside the current Emacs
>> ;; process. When this occurs the metadata of the fragment can
>> ;; still exist in `org-persist--index', but the image file is
>> ;; gone. This condition can be detected by checking if the
>> ;; `cadr' is nil (indicating the image has gone AWOL).
>> (if (cadr label-path-info)
>> (cons (cadr label-path-info)
>> (caddr label-path-info))
>> (org-latex-preview--remove-cached key)
>> nil)
>
> 1. you are calling org-latex-preview--remove-cached without passing
> cache-location
Added cache-location:
(org-latex-preview--remove-cached key cache-location)
> 2. This seems redundant and may be merged into
>> (if (and result (file-exists-p (car result)))
>> result
>> (org-latex-preview--remove-cached key cache-location)
>> nil)
I'm not seeing how. I removed the extra call to
(org-latex-preview--remove-cached key cache-location)
but don't see a way to simplify it further.
>> (dolist (collection org-persist--index)
>> (when (equal (cadar (plist-get collection :container))
>> org-latex-preview--cache-name)
>> (org-latex-preview--remove-cached
>> (plist-get (plist-get collection :associated) :key)
>> 'persist)
>> (cl-incf n)))
>
> It should be possible to search this via org-persist itself. Please add
> FIXME for future feature to add.
Added a FIXME.
>> (end (- (org-element-property :end element)
>> (or (org-element-property :post-blank element)
>> 0)
>> (if (eq (char-before (org-element-property :end
>> element))
>> ?\n)
>> 1 0)))
>
> same issue with calculation of before-blank point.
I've now replaced all instances of this calculation:
(- (org-element-property :end element)
(or (org-element-property :post-blank element) 0)
(if (eq (char-before (org-element-property :end element))
?\n)
1 0))
with this:
(org-with-wide-buffer
(goto-char (org-element-property :end element))
(skip-chars-backward "\n\r\t ")
(point))
I couldn't find a way to do it with just org-element. If you know of a
better way please let me know.
>> (defun org-latex-preview--attr-color (attr)
>> "Return a RGB color for the LaTeX color package."
>
> checkdoc.
Addressed in an earlier commit.
> With this, I went through all the changes, except unrelated to the latex
> preview branch.
Thanks. I think I've addressed all feedback related to
org-latex-preview.el itself (not ox-html, ox-odt etc) except for
- %O placeholder in org-latex-preview-process-alist
- Redesign of org-latex-preview-live--src-buffer-setup
