[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] (v2) New LaTeX code export option: engraved
From: |
Timothy |
Subject: |
Re: [PATCH] (v2) New LaTeX code export option: engraved |
Date: |
Mon, 09 May 2022 20:57:12 +0800 |
User-agent: |
mu4e 1.6.10; emacs 28.0.92 |
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
- Re: [PATCH] New LaTeX code export option: engraved, (continued)
- Re: [PATCH] New LaTeX code export option: engraved, Daniel Fleischer, 2022/05/05
- Re: [PATCH] New LaTeX code export option: engraved, Ihor Radchenko, 2022/05/05
- Re: [PATCH] New LaTeX code export option: engraved, Timothy, 2022/05/05
- Re: [PATCH] New LaTeX code export option: engraved, Timothy, 2022/05/05
- Re: [PATCH] New LaTeX code export option: engraved, Ihor Radchenko, 2022/05/07
- Re: [PATCH] New LaTeX code export option: engraved, Timothy, 2022/05/07
- Re: [PATCH] New LaTeX code export option: engraved, Timothy, 2022/05/07
- Re: [PATCH] New LaTeX code export option: engraved, Daniel Fleischer, 2022/05/07
- Re: [PATCH] (v2) New LaTeX code export option: engraved, Timothy, 2022/05/08
- Re: [PATCH] (v2) New LaTeX code export option: engraved, Ihor Radchenko, 2022/05/09
- Re: [PATCH] (v2) New LaTeX code export option: engraved,
Timothy <=
- Re: [PATCH] (v2) New LaTeX code export option: engraved, Max Nikulin, 2022/05/10
- Re: [PATCH] (v2) New LaTeX code export option: engraved, Ihor Radchenko, 2022/05/11
- Re: [PATCH] (v3) New LaTeX code export option: engraved, Timothy, 2022/05/11
- Re: [PATCH] (v3) New LaTeX code export option: engraved, Daniel Fleischer, 2022/05/12
- Re: [PATCH] (v3) New LaTeX code export option: engraved, Timothy, 2022/05/12
- Re: [PATCH] New LaTeX code export option: engraved, Max Nikulin, 2022/05/05
- Re: [PATCH] New LaTeX code export option: engraved, Ihor Radchenko, 2022/05/07
Re: [PATCH] New LaTeX code export option: engraved, Sébastien Miquel, 2022/05/09