From: Timothy <firstname.lastname@example.org> To: Ihor Radchenko <email@example.com> Cc: Daniel Fleischer <firstname.lastname@example.org>, email@example.com, Nicolas Goaziou <firstname.lastname@example.org> Subject: Re: [PATCH] (v2) New LaTeX code export option: engraved Date: Mon, 09 May 2022 20:57:12 +0800 [thread overview] Message-ID: <email@example.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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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).