Timothy <tecos...@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
