From: Ihor Radchenko <yantar92@gmail.com>
To: Timothy <tecosaur@gmail.com>
Cc: Daniel Fleischer <danflscr@gmail.com>,
emacs-orgmode@gnu.org, Nicolas Goaziou <mail@nicolasgoaziou.fr>
Subject: Re: [PATCH] (v2) New LaTeX code export option: engraved
Date: Mon, 09 May 2022 14:20:44 +0800 [thread overview]
Message-ID: <874k1zqlab.fsf@localhost> (raw)
In-Reply-To: <87levcm2bx.fsf@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
next prev parent reply other threads:[~2022-05-09 6:20 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-04 15:59 [PATCH] New LaTeX code export option: engraved Timothy
2022-05-05 7:52 ` Daniel Fleischer
2022-05-05 16:09 ` Timothy
2022-05-06 2:35 ` Ihor Radchenko
2022-05-06 11:23 ` Timothy
2022-05-05 7:54 ` Daniel Fleischer
2022-05-05 8:48 ` Ihor Radchenko
2022-05-05 15:17 ` Timothy
2022-05-05 16:13 ` Timothy
2022-05-07 5:16 ` Ihor Radchenko
2022-05-07 6:57 ` Timothy
2022-05-07 10:40 ` Timothy
2022-05-07 11:33 ` Daniel Fleischer
2022-05-08 14:30 ` [PATCH] (v2) " Timothy
2022-05-09 6:20 ` Ihor Radchenko [this message]
2022-05-09 12:57 ` Timothy
2022-05-10 8:00 ` Max Nikulin
2022-05-11 11:06 ` Ihor Radchenko
2022-05-11 16:05 ` [PATCH] (v3) " Timothy
2022-05-12 16:40 ` Daniel Fleischer
2022-05-12 16:44 ` Timothy
2022-05-05 16:48 ` [PATCH] " Max Nikulin
2022-05-07 4:13 ` Ihor Radchenko
2022-05-09 19:19 ` Sébastien Miquel
2022-05-10 1:13 ` Timothy
2022-05-10 16:10 ` Timothy
2022-05-10 17:53 ` Sébastien Miquel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874k1zqlab.fsf@localhost \
--to=yantar92@gmail.com \
--cc=danflscr@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=mail@nicolasgoaziou.fr \
--cc=tecosaur@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).