From: Timothy <tecosaur@gmail.com>
To: Ihor Radchenko <yantar92@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 20:57:12 +0800 [thread overview]
Message-ID: <87ee12n6re.fsf@gmail.com> (raw)
In-Reply-To: <874k1zqlab.fsf@localhost>
[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]
Hi Ihor,
> 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.
Probably not a bad idea, I’m just not sure what/how I’d go about this.
> Also, unless I miss something, there is no support for coderefs in the
> patchset. Is it intentional?
I think you’ve missed something. It has the same coderefs support as minted.
> It appears that the code for caption-str is duplicated. It could be
> also factored out.
It could, but I’m not sure this is actually a good idea as the duplication is
more the result of chance than a common dependency.
>> + (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.
This code isn’t introduced by my patches, just relocated. FWIW `format-spec' is
provided by Emacs, and I don’t think is new.
>> + (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.
Done.
>> +% In case engrave-faces-latex-gen-preamble has not been run.
>> +\
>> +\
>
> What is the difference between EfD and EFD? EfD is also not documented.
Documentation added. EfD is the background colour.
>> +FVEXTRA-SETUP sets fvextra’s defaults according to
>> +`org-latex-engraved-options’, and LISTINGS-SETUP creates the
>> +listings environment and defines \.“
>
> 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.
It does the same thing as minted’s `listings' environment, hence the choice of
name. Documentation added.
> Why not just
> (org-element-map (plist-get info :parse-tree)
> ’(src-block inline-src-block example-block fixed-width) #’identity
> info t)
Ah, in my config I’m also using some engraved info for example/fixed-width
blocks. I’ll leave this out for now.
>> (pcase (plist-get info :latex-listings)
>> (’nil (org-latex–text-markup code ’code info))
>> …
> Same comment about ` in pcase.
Done.
> What will happen if there is no [LISTINGS-SETUP]?
Captioned/Floating code blocks won’t work.
>> +(defcustom org-latex-engraved-theme nil
> Docstring does not explain what happens when set to nil.
> Also, does :type ’symbol allow t and nil?
I think so, `symbolp' says they’re symbols if nothing else.
> Will it work when engraved-theme is t?
Yes.
> 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?
Resolved earlier.
>> + (gen-theme-spec
>> + (lambda (theme) …
>
> This appears to be not guarded by (require ’engrave-faces-latex nil t).
It is before it’s actually called.
>> -(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.
Now documented.
I have checked compile warnings (there are none), but checkdoc is currently a
bit dodgy on the Emacs commit I’m on.
>> - (insert content)
>> + (insert (string-trim-right content “”))
>
> As a theoretical possibility, what will happen if content has something
> like “%”?
Discussed in DMs. It just gets rid of trailing newlines, and it’s fine with
escaping.
All the best,
Timothy
next prev parent reply other threads:[~2022-05-09 14:06 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-04 15:59 [PATCH] " 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
2022-05-09 12:57 ` Timothy [this message]
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=87ee12n6re.fsf@gmail.com \
--to=tecosaur@gmail.com \
--cc=danflscr@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=mail@nicolasgoaziou.fr \
--cc=yantar92@gmail.com \
--subject='Re: [PATCH] (v2) New LaTeX code export option: engraved' \
/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
Code repositories for project(s) associated with this 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).