Pedro Andres Aranda Gutierrez <[email protected]> writes:

> Subject: [PATCH 1/2] LaTeX export: add org-latex-fontspec-config

Thanks for the updated patch. The new wording on the manual reads much
smoother compared to the early versions.

Some minor comments inline.

> * doc/org-manual.org
> (*** Controlling font setup for LuaLaTeX andXeLaTeX): New section
> describing org-latex-fontspec-config.

You should just use "(Controlling font setup for LuaLaTeX andXeLaTeX)"
to refer to section.
Also, 'org-latex-fontspec-config' (quoted).

> * etc/ORG-NEWS: Annouce org-latex-fontspec-config.

'org-latex-fontspec-config'.
And you missed the section in ORG-NEWS where you put the announcement.

> (org-latex-guess-fontspec): New funciton to insert the fontspec
> configuration when the fontspec package is requested in the LaTeX
> prelude.

Shouldn't "prelude" be preamble?

> (org-latex-make-preamble): Add the previous function in the chain of
> guessers, just before the babel and polyglossia guessers. Fix null class
> options or empty string.

Double space between sentences.

> (test-ox-latex/lualatex-fontspec-recognised): Test correct
> implementation of fontspec handling.
> (test-ox-latex/lualatex-fontspec-recognised): Test fallbacks.

You have a duplicate changelog entry here.

> +This mapping takes place in ~org-latex-fontspec-config~.  Although not
> +mandatory, it is recommended that you define mapping for the four
> +families.  An example mapping the fonts to use another popular font family,
> +[[https://www.gust.org.pl/projects/e-foundry/tex-gyre][the TeX Gyre
> +(TG) Collection of Fonts]] could be [fn::We use this family, because
> +it is also included in most LaTeX distributions.]:

Nit: Maybe fn can go right after "the TeX Gyre (TG) Collection of
Fonts", not after "could be".

> +As you see, each element in the list maps one of the LaTeX font
> +families to a property =:font=, that indicates the font to use for the

~:font~ (it is a code)

> +Additionally, you can also define "=:features="[fn::You may also use
> +=:props= as a synonym for =:features=.] for each of the fonts.

~:features~, ~:props~.

> +the missing glyph set, by using the =:fallback= property for the

~:fallback~ (similar problems later on as well)

> +*** New custom variable ~org-latex-fontspec-config~
> +
> +When using LuaLaTeX (or XeLaTeX in a reduced scope), you can use the
> +new custom variable ~org-latex-fontspec-config~ to change the
> +document's fonts and add fallback fonts for missing scripts.

I think we can refer to the new section in the manual for more details.

>    "Return complete document string after Beamer conversion.
>  CONTENTS is the transcoded contents string.  INFO is a plist
>  holding export options."
> +  ;; Before doing anything else, add the script information
> +  ;; to the INFO channel. Used by org-latex-make-preamble
> +  ;; to add fallback fonts for lualatex.
> +  (setq info (plist-put info
> +                        :doc-scripts
> +                        (org-get-string-scripts contents)))
> +

May we do the same in :filter-body instead? Then, we can just arrange
calling org-get-string-scripts in ox-latex without a need to repeat the
procedure in ox-beamer.

> +(defcustom org-latex-fontspec-config nil
> +  "An alist with the configuration for the fontspec package.
> +
> +This configuration will be generated when using lualatex or xelatex.
> +
> +Each element is defined as
> +(`font-name' . `font-plist')
> + where `font-name' one of \"main\", \"sans\", \"mono\" or \"math\"
> + and `font-plist' is a plist. The keys for this plist are
> +  `:font':     font name for font installed in your system
> +  `:features': string or list of strings with font features (optional).
> +  `:fallback': an alist of (`script' . `mapping') to map _Emacs_ script names
> +               to their fallback font (optional). The exporter will warn you
> +               about scripts in your document that need a fallback font."

These are not many details.
We should at least refer to the new manual section.

> +  :group 'org-export-latex
> +  :package-version '(Org . "10.0")
> +  :type 'alist

We could use a more detailed type definition.

> +(defun org-latex-guess-fontspec (header info)
> +  "Add the fontspec package configuration passed in INFO to HEADER.
> +
> +The HEADER contains \"\\usepackage{fontspec}\",
> +and INFO contains fontspec font conguration and
> +add the fontspec configuration after the package."
> +    (when-let* ((fonst (plist-get info :latex-fontspec-config))

fonst?

> +                (matched (string-match "\\\\usepackage{fontspec}\n" header))

Why trailing \n?
Also, what about \usepackage[...]{fontspec}?

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Reply via email to