Re: [O] (V8) [PATCH] New feature: Use dvisvgm to preview latex formular

[Nicolas Goaziou]

Hello,

"Feng Shu" <address@hidden> writes:

> I have rebase my patch to new function org-compile-file, the v8 patch is
> very different with the earlier version, it is more simpler i think,
> please comment again, thanks for your help!

Thank you. I added `org-compile-file' to master branch and incorporated
some of you suggestions. Beware, however, that there are some
differences with the version you're using here. In particular, 

1. the order of arguments is different;
2. %b, %f, and %o are always replaced in commands, no need to specify
   them again when using argument SPEC;
3. extension argument is a string, not a symbol or a list of symbols.

By the way, I don't understand why you do need to provide a list of
extensions. The extension argument is used to check if the process
actually succeeded, but the process itself can create as many files as
it sees fit.

Some additional comments follow.

> +    (dvisvgm
> +     :programs ("latex" "dvisvgm" "gs")
> +     :message "you needed to install latex, dvisvgm and ghostscript."
> +     :use-xcolor t
> +     :image-input (dvi xdv)

Per above, it is either dvi or xdv

> +  "List definitions of external processes for LaTeX previewing.
> +Org mode can use some external commands to generate TeX snippet's image for
> +previewing or inserting into HTML files, e.g. dvipng, dvisvgm or imagemagick.
> +This variable tells `org-create-formula-image' how to use external commands.
> +
> +  :name               symbol, the process setting name.
> +  :inherit            symbol, inhert options from an exist process setting.

I suggest to drop the property above. It is only partially implemented
(e.g., you cannot inherit from an inherited process), it complicates
code, and I don't think it is terribly useful overall.

> +  :image-input        symbol, input file type, for example: dvi.
> +  :image-output       symbol, output file type, for example: png.

symbol -> string

> +  :use-xcolor         boolean, if set to `t', LaTeX \"xcolor\" macro is used
> +                      to deal with background and foreground color of image,
> +                      if set to `nil', dvipng style background and foregroud 
> color
> +                      format are generated; you should used them in command 
> options
> +                      with special string: \"%F\" and \"%B\".

when non-nil, `LaTeX' \"xcolor\"... color of image.  Otherwise, dvipng
style... are generated.  You may then refer to them in command options
with "%F" and "%B".

> +  :image-size-adjust  cons of numbers, the car element is used to adjust 
> latex image

latex -> LaTeX

> +                      size showed in buffer and the cdr element is for html 
> file.

html -> HTML

> +                      This option is only useful for backend developers, 
> users
> +                      should use variable `org-format-latex-options' instead.
> +  :post-clean         list of strings, files matched are to be cleaned up 
> once the
> +                      image is generated. If set to `nil', the files with 
> dvi, xdv,
> +                      pdf, tex, aux, log, svg, png, jpg, jpeg or out 
> extension will
> +                      be cleaned up.
> +  :latex-header       list of string, latex snippet file's header, if set to 
> `nil',

latex -> `LaTeX'

> +                      the return value of `org-create-formula--latex-header' 
> will be
> +                      used, which is controled `org-format-latex-header',
> +                      `org-latex-default-packages-alist' and 
> `org-latex-packages-alist'.

No need to reference an internal function:

  the return value is controlled by `org-format-latex-header'
  `org-latex-default-packages-alist' and `org-latex-packages-alist',
  which see.

> +  :latex-compiler     list of latex commands, each of them will be given to 
> the shell
> +                      as a command.  the special strings, %t, %b and %o, 
> will be replaced
> +                      to according value before commands called.

as a command.  Place-holders "%t", "%b" and "%o" are replaced with
values defined below.

> +  :image-converter    list of image converter command strings, each of them 
> will be
> +                      given to the shell as a command.  the following 
> special strings,
> +                      will be replaced to according value before commands 
> called.

each of them is given to the shell as a command and support any of the
following place-holders defined below.

> +Special strings used by `:image-converter' and `:latex-compiler':

Place-holders used by...

> +5. %t    tex file name.

Couldn't you use %f so as to be compatible with default place-holders?

> -(defun org--format-latex-make-overlay (beg end image)
> +(defun org--format-latex-make-overlay (beg end image &optional imagetype)
>    "Build an overlay between BEG and END using IMAGE file."

You need to explain IMAGETYPE in docstring, e.g.,

  Argument IMAGETYPE is the extension of the displayed image, as
  a symbol. It defaults to "png".

> +                (let* ((processing-info
> +                        (cdr (assq processing-type 
> org-preview-latex-process-alist)))
> +                       (inherit-processing-info
> +                        (cdr (assq (plist-get processing-info :inherit)
> +                                   org-preview-latex-process-alist)))
> +                       (inherit-processing-info
> +                        (if (or (plist-get inherit-processing-info :inherit)
> +                                (eq (plist-get processing-info :inherit) 
> type))
> +                            (progn (error "'%s' inherit an invalid process 
> setting." processing-type) nil)
> +                          inherit-processing-info))

Again, I suggest to drop the part about inheriting process info. 

If you think it is a must-have, then it should be factored out of this
function and from `org-create-formula-image', as there is too much code
duplication. It should also support inheriting from inherited processes.

> +                       (face (face-at-point))
>                         ;; Get the colors from the face at point.
>                         (fg
>                          (let ((color (plist-get org-format-latex-options
> @@ -19172,9 +19271,12 @@ Some of the options can be changed using the variable
>                                            org-latex-packages-alist
>                                            org-format-latex-options
>                                            forbuffer value fg bg))))
> +                       (imagetype (or (plist-get processing-info 
> :image-output)
> +                                      (plist-get inherit-processing-info 
> :image-output)
> +                                      'png))

Ditto.

> +(defun org-create-formula-image (string tofile options buffer &optional type)
> +  "Create an image from LaTeX source using external processes.
> +
> +The external processes are defined in `org-preview-latex-process-alist'."

Ideally, each argument need to be explained in the docstring. I know the
previous function didn't do that, but, if you can improve it, that's
better.

>    (require 'ox-latex)
> -  (let* ((tmpdir (if (featurep 'xemacs)
> -                  (temp-directory)
> -                temporary-file-directory))
> +  (let* ((type (or type 'dvipng))
> +      (processing-info
> +       (cdr (assq type org-preview-latex-process-alist)))
> +      (inherit-processing-info
> +       (cdr (assq (plist-get processing-info :inherit)
> +                  org-preview-latex-process-alist)))
> +      (inherit-processing-info
> +       (if (or (plist-get inherit-processing-info :inherit)
> +               (eq (plist-get processing-info :inherit) type))
> +           (progn (error "'%s' inherit an invalid process setting." type) 
> nil)
> +         inherit-processing-info))
> +      (programs
> +       (or (plist-get processing-info :programs)
> +           (plist-get inherit-processing-info :programs)))
> +      (error-message
> +       (or (plist-get processing-info :message)
> +           (plist-get inherit-processing-info :message)))
> +      (use-xcolor
> +       (or (plist-get processing-info :use-xcolor)
> +           (plist-get inherit-processing-info :use-xcolor)))
> +      (image-input-types
> +       (or (plist-get processing-info :image-input)
> +           (plist-get inherit-processing-info :image-input)))
> +      (image-output-type
> +       (or (plist-get processing-info :image-output)
> +           (plist-get inherit-processing-info :image-output)))
> +      (post-clean (or (plist-get processing-info :post-clean)
> +                      (plist-get inherit-processing-info :post-clean)
> +                      '(".dvi" ".xdv" ".pdf" ".tex" ".aux" ".log"
> +                        ".svg" ".png" ".jpg" ".jpeg" ".out")))
> +      (latex-header (or (plist-get processing-info :latex-header)
> +                        (plist-get inherit-processing-info :latex-header)
> +                        (org-create-formula--latex-header)))
> +      (latex-compiler
> +       (or (plist-get processing-info :latex-compiler)
> +           (plist-get inherit-processing-info :latex-compiler)))
> +      (image-converter
> +       (or (plist-get processing-info :image-converter)
> +           (plist-get inherit-processing-info :image-converter)))

This is the repetitive pattern I was talking about.

> +    (setq image-input-file
> +       (org-compile-file
> +        texfile image-input-types latex-compiler log-buf
> +        (format "Please adjust '%s' part in 
> `org-preview-latex-process-alist'." type)
> +        `((?t . ,(shell-quote-argument texfile))
> +          (?b . ,(shell-quote-argument texfilebase))
> +          (?o . ,(shell-quote-argument tmpdir)))))

As explained above, beware the order of arguments and the type of
IMAGE-OUTPUT-TYPE. Also, I don't think you need to provide SPEC, as it
is the default, with a slight change ?t is actually ?f.

> +    (setq image-output-file
> +       (org-compile-file
> +        image-input-file image-output-type image-converter log-buf
> +        (format "Please adjust '%S' part in 
> `org-preview-latex-process-alist'." type)
> +        `((?F . ,(shell-quote-argument fg))
> +          (?B . ,(shell-quote-argument bg))
> +          (?d . ,(shell-quote-argument (format "%s" dpi)))
> +          (?s . ,(shell-quote-argument (format "%s" (/ dpi 140.0))))
> +          (?t . ,(shell-quote-argument texfile))
> +          (?i . ,(shell-quote-argument image-input-file))
> +          (?p . ,(shell-quote-argument image-output-file))
> +          (?b . ,(shell-quote-argument texfilebase))
> +          (?o . ,(shell-quote-argument tmpdir)))))

As explained above, beware the order of arguments and the type of
IMAGE-OUTPUT-TYPE. Also, you don't need to provide ?o, ?b and
probably ?t in SPEC.

> +    (when (file-exists-p image-output-file)

IMAGE-OUTPUT-FILE is guaranteed to exist.  Otherwise, `org-compile-file'
returns an error.

> +(defun org-compile-file (source extensions process
> +                                &optional log-buffer message spec)

You can remove this, as it is integrated in master.
> -`dvipng'       Process the LaTeX fragments to images.  This will also
> +`dvipng'       Process the LaTeX fragments to png images.  This will also
> +               include processing of non-math environments.
> +`dvisvgm'      Process the LaTeX fragments to svg images.  This will also
>                 include processing of non-math environments.
>  `imagemagick'  Convert the LaTeX fragments to pdf files and use
>                 imagemagick to convert pdf files to png files.

Process types are mostly hard-coded, this is a reason why I don't think
inheritance is terribly useful: you almost always want to modify
existing processes, not create new ones.

I think we're getting close to the merge.

Regards,

-- 
Nicolas Goaziou