Karthik Chikmagalur <[email protected]> writes:

> The updated version of the manual and ORG-NEWS are now pushed to Teco's
> fork.  These include the naming changes we discussed over voice call
> (and documented in https://list.orgmode.org/[email protected]).
>
> Most of your suggestions have been incorporated.  Please review it when
> possible

Thanks! And sorry for the late review. A combination of work travel,
family matters, and me being sick delayed things more than I
anticipated.

Below are some minor comments on the latest version of the
new documentation.

> Org mode provides two levels of automatic LaTeX preview generation
> with increasing degrees of immediate feedback:

This sounds abrupt. The paragraph starts right after describing the
basic preview functionality, but it is not immediately obvious that
"automatic LaTeX preview" is referring to an entirely new feature.
Maybe we can add a bit of the context like: "Normally, latex fragments
previews visually replace the underlying latex text. Editing the latex
fragments thus requires disabling the preview first. To make editing
easier, Org also provides optional /automatic/ previews ..."

> - Live previews: Continuously preview LaTeX fragments as you type,
>   giving you near immediate feedback (especially when using
>   =pdflatex=).  The preview image will continue to be displayed
>   adjacent to the fragment even when editing it.

It is worth explaining what happens when the fragment has errors or does
not compile.

> - (~org-latex-preview-reveal-mode~) ::
                        ^^^^^^
>   #+findex: org-latex-preview-mode

> #+vindex: org-latex-preview-mode-ignored-environments
> You can specify a list of LaTeX environments that should
> not be automatically previewed by ~org-latex-preview-mode~ when
> they are inserted into the buffer.  This is controlled by
> ~org-latex-preview-mode-ignored-environments~.

It is worth mentioning that the default value ignores figure
environments. (BTW, what will happen if somebody removes figure from
that list?)

> #+vindex: org-latex-preview-debounce
> By default, there is a delay (debounce) of 1 second between editing a
> fragment and generating a live preview.  This can be changed by
> setting ~org-latex-preview-debounce~.  Note that values of 0.1 or
> lower can cause high CPU utilization.

`org-latex-preview-mode-update-delay'?

Now, I think that we are more or less done with the manual and I will go
ahead with reviewing the code.

Some initial comments.

> (defvar org-latex-preview--dvisvgm3-minor-version
>   (or (and (executable-find "dvisvgm")
>            (with-temp-buffer
>              (call-process "dvisvgm" nil t nil "--version")
>              (let ((ver (version-to-list
>                          (string-trim (buffer-string) "dvisvgm "))))
>                (and (= (car ver) 3) (cadr ver)))))
>       -1)
>   "The minor version of dvisvgm, if dvisvgm 3 is installed.  Otherwise -1.")

I am not sure if I like calling dvisvgm as a side effect of loading the
library. I recommend a different route instead: write a helper
function org-latex-preview--dvisvgm3-minor-version that will either
return the variable value or, if the variable is nil, compute the value
only when it is really needed.

> (defcustom org-latex-preview-appearance-options
>   '(:foreground auto :background "Transparent"
>     :scale 1.0 :zoom 1.0 :page-width 0.6
>     :matchers ("begin" "$1" "$" "$$" "\\(" "\\["))

I am wondering if we should take this opportunity to remove :matchers
from this customization into some dedicated option.

> :matchers    A list indicating which matchers should be used to
>              find LaTeX fragments.  Valid members of this list are:
>              \"begin\" find environments
>              \"$1\"    find single characters surrounded by $.$
>              \"$\"     find math expressions surrounded by $...$
>              \"$$\"    find math expressions surrounded by $$....$$
>              \"\\(\"    find math expressions surrounded by \\(...\\)
>              \"\\=\\[\"    find math expressions surrounded by \\=\\[...\\]"

... because the docstring is not accurate anymore and creates an
impression that :matchers is used for the purposes of preview, not for
underlying latex text fontification.

> :type 'plist

Since we are obsoleting :html-* elements of this plist, it will be a
good idea to provide a proper specifier with a closed list of allowed
keywords here, so that people get more hints when they are using
obsolete value.

>      (dvisvgm
>       :programs ("latex" "dvisvgm")
>       :description "dvi > svg"
>       :message "you need to install the programs: latex and dvisvgm."
>       :image-input-type "dvi"
>       :image-output-type "svg"
>       :latex-compiler ("%l -interaction nonstopmode -output-directory %o %f")
>       :latex-precompiler ("%l -output-directory %o -ini -jobname=%b \"&%L\" 
> mylatexformat.ltx %f")
>       ;; The --optimise, --clipjoin, and --relative flags cause dvisvgm to
>       ;; do some extra work to tidy up the SVG output, but barely add to
>       ;; the overall dvisvgm runtime (<1% increace, from testing).
>       :image-converter determined-at-runtime)

The comment seems misplaced.
Also, determined-at-runtime is not documented.

> ;; This is a bit hacky, but since we can't include reference to the
> ;; runtime value of `org-latex-preview--dvisvgm3-minor-version' in the
> ;; default value of `org-latex-preview-process-alist', we have to
> ;; resort to modifying the value at runtime like so.
> ;; Theoretically only the "load" condition is needed, but some people seemed
> ;; to have problems with this that are solved by adding "eval".
> (cl-eval-when (load eval)
>   (when-let ((dvisvgm (alist-get 'dvisvgm org-latex-preview-process-alist)))
>     (when (eq (plist-get dvisvgm :image-converter) 'determined-at-runtime)
>       (plist-put dvisvgm :image-converter
>                  (list
>                   (concat "dvisvgm --page=1- --optimize --clipjoin --relative 
> --no-fonts"
>                           (if (>= org-latex-preview--dvisvgm3-minor-version 2)
>                               " -v3 --message='processing page {?pageno}: 
> output written to {?svgpath}'" "")
>                           " --bbox=preview -o %B-%%9p.svg %f"))))))

You can simply process that 'determined-at-runtime when we access the
variable, replacing it with relevant value as needed. Similar idea to
`org-latex-preview--dvisvgm3-minor-version' computed using a helper
function.

> (cl-defun org-async-call (proc &key success failure filter buffer info 
> timeout now process-variables
>                                (dir default-directory) (coding 'utf-8))

I believe that this new functionality should be mentioned in ORG-NEWS.
For the function itself, it would also be nice to have some examples of
usage, so that people not familiar with preview systems were able to
make use of this queuing functionality.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
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>

Reply via email to