emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
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


  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] " 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 \
    --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).