Re: [PATCH] (v2) New LaTeX code export option: engraved

[Ihor Radchenko <yantar92@gmail.com>]

Timothy <tecosaur@gmail.com> writes:

> I have a new set of patches (attached), which make use of a major new feature in
> engrave-faces v0.3: engraved themes.

Thanks!

> Glad to hear you’ve been able to have a look over the patches. With the feedback
> I’ve received here so far, if no issues come up in the next few days I’m
> inclined to merge this, add documentation, and see what feedback pops
> up.

Before merging, could you also try to implement tests at least for
engraved feature? If I recall correctly, we do not currently have
backend-specific tests. But it would certainly help if we did. You might
as well write a small "nucleus" test for ox-latex.

Also, unless I miss something, there is no support for coderefs in the
patchset. Is it intentional?

> +(defun org-latex-src-block--verbatim
> ...
> +  (let ((caption-str (org-latex--caption/label-string src-block info))

> +(defun org-latex-src-block--custom
> ...
> +  (let ((caption-str (org-latex--caption/label-string src-block info))
> +        (formatted-src (org-export-format-code-default src-block info)))

It appears that the code for caption-str is duplicated.  It could be
also factored out.

> +      (format-spec custom-env
> +                   `((?s . ,formatted-src)
> +                     (?c . ,caption)
> +                     (?f . ,float)
> +                     (?l . ,(org-latex--label src-block info))
> +                     (?o . ,(or (plist-get attributes :options) "")))))))

I do not see a definition of `format-spec' function. There is only
`spec' above in the code. Can you double check? Either I am missing
something or something fishy is going on.

> +  (let ((code (org-element-property :value inline-src-block))
> +        (lang (org-element-property :language inline-src-block)))
> +    (pcase (plist-get info :latex-listings)
> +      ('nil (org-latex--text-markup code 'code info))
> +      ('minted (org-latex-inline-src-block--minted info code lang))
> +      (_ (org-latex-inline-src-block--listings info code lang)))))

Please use `nil and `minted.  Using ' may create issues in older Emacs.

> +% In case engrave-faces-latex-gen-preamble has not been run.
> +\\providecolor{EfD}{HTML}{f7f7f7}
> +\\providecolor{EFD}{HTML}{28292e}

What is the difference between EfD and EFD? EfD is also not documented.


> +FVEXTRA-SETUP sets fvextra's defaults according to
> +`org-latex-engraved-options', and LISTINGS-SETUP creates the
> +listings environment and defines \\listoflistings."

It is unclear what listings environment does given that we use engraved
package. Can you provide a bit more details in the docstring on what the
user will get if [LISTINGS-SETUP] is present.
It may catch users by surprise that deleting this will make captions
disappear.

> @@ -1756,6 +1929,17 @@ (defun org-latex-template (contents info)
>       (let ((template (plist-get info :latex-hyperref-template)))
>         (and (stringp template)
>              (format-spec template spec)))
> +     ;; engrave-faces-latex preamble
> +     (when (eq org-latex-listings 'engraved)
> +       (let ((src-p (org-element-map (plist-get info :parse-tree)
> +                        '(src-block inline-src-block) #'identity
> +                        info t))
> +             (fixedw-p
> +              (org-element-map (plist-get info :parse-tree)
> +                  '(example-block fixed-width) #'identity
> +                  info t)))
> +         (when (or src-p fixedw-p)
> +           (org-latex-generate-engraved-preamble info src-p))))

Why not just
(org-element-map (plist-get info :parse-tree)
                        '(src-block inline-src-block example-block fixed-width) #'identity
                        info t)
?

>      (pcase (plist-get info :latex-listings)
>        ('nil (org-latex--text-markup code 'code info))
>        ('minted (org-latex-inline-src-block--minted info code lang))
> +      ('engraved (org-latex-inline-src-block--engraved info code lang))
>        (_ (org-latex-inline-src-block--listings info code lang)))))

Same comment about ` in pcase.
  
>  (defun org-latex-inline-src-block--listings (info code lang)
>    "Transcode an inline src block's content from Org to LaTeX, using lstlistings.
>  INFO, CODE, and LANG are provided by `org-latex-inline-src-block'."
> @@ -2323,6 +2513,7 @@ (defun org-latex-keyword (keyword _contents info)
>  	  (cl-case (plist-get info :latex-listings)
>  	    ((nil) "\\listoffigures")
>  	    (minted "\\listoflistings")
> +	    (engraved "\\listoflistings")
>  	    (otherwise "\\lstlistoflistings")))))))))

What will happen if there is no [LISTINGS-SETUP]?

> +(defcustom org-latex-engraved-theme nil
> +  "The theme that should be used for engraved code, when non-nil.
> +This can be set to any theme defined in `engrave-faces-themes' or
> +loadable by Emacs.  When set to t, the current Emacs theme is
> +used."
> +  :group 'org-export-latex
> +  :type 'symbol)

Docstring does not explain what happens when set to nil.
Also, does :type 'symbol allow t and nil?

> -             (engrave-faces-latex-gen-preamble)
> +             (engrave-faces-latex-gen-preamble
> +              (when engraved-theme (intern engraved-theme)))

Will it work when engraved-theme is t?

> -         (engraved-theme (plist-get info :latex-engraved-theme)))
> +         (engraved-theme (plist-get info :latex-engraved-theme))
> +         (engraved-themes
> +          (cl-delete-duplicates
> +           (org-element-map
> +               (plist-get info :parse-tree)
> +               '(src-block inline-src-block)
> +             (lambda (src)
> +               (plist-get
> +                (org-export-read-attribute :attr_latex src)
> +                :engraved-theme))
> +             info)))

What about example-block and fixed-width? I may be missing something,
but earlier in the patchset you had conditionals of the type
(or src-p fixedw-p). So, does engrave-faces do anything with fixedw-type
elements?

> +         (gen-theme-spec
> +          (lambda (theme)
> +            (if (eq engrave-faces-latex-output-style 'preset)
> +                (engrave-faces-latex-gen-preamble (when theme (intern theme)))
> +              (engrave-faces-latex-gen-preamble-line
> +               'default
> +               (alist-get 'default
> +                          (if theme
> +                              (engrave-faces-get-theme (intern theme))
> +                            engrave-faces-current-preset-style)))))))

This appears to be not guarded by (require 'engrave-faces-latex nil t).

> -(defun org-latex-src--engrave-code (content lang)
> -  "Engrave CONTENT to LaTeX in a LANG-mode buffer, and give the result."
> +(defun org-latex-src--engrave-code (content lang &optional theme options inline)
> +  "Engrave CONTENT to LaTeX in a LANG-mode buffer, and give the result.
> +When THEME is non-nil, it will be used."

You did not document the remaining two arguments. (this makes me ask a
question whether you checked compile warnings). Also, consider running
M-x checkdoc on ox-latex.el.

> -                (insert content)
> +                (insert (string-trim-right content "\n"))

As a theoretical possibility, what will happen if content has something
like "%\n"?

Best,
Ihor