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

> OK further experiments I have done show that
> if there is a \setCJK...font{} declaration (for example from a
> .dir-locals.el),
> you need xeCJK. Therefore we will keep the code as is.
> If we do not add this package in the presence of explicit CJK fonts, we
> don't produce compilable documents.

      ;; If the CJK font families have been included
      ;; Check for polyglossia and/or babel and warn?
      ;; Or advise for these packages to be added to `org-latex-package-alist' 
??
      (when (and cjk-packages (equal compiler "xelatex"))
        (message "Adding the CJK packages")
        (goto-char (point-min))
        (forward-line 2)
        (insert "\\usepackage[CJKspace]{xeCJK}\n"))

Then, can you update the comment explaining why xeCJK is necessary?

> Could we keep that as a FIXME for a refinement and discuss it further in
> the future?

Sure.

>> It is right from
>> 包时,org当中的加粗转为中间文件*tex*时是 - 200B*tex*200B escaping
>> emphasis markup, as full with spaces are not used in Chinese writing.
>
> And it appears when the default font is lmroman.
> Setting the main font to 'Noto Serif' got rid of the message.
>
> The other option is the LATEX_HEADER in your proposal plus
> #+LATEX: \ctexset{space=auto}
> which I got from
> https://emacs-china.org/t/help-implementing-better-out-of-the-box-xelatex-export-in-org/30405/6
> (thanks LuciusChen for this!)
> With that, using lmroman will not trigger the warning and it will not
> interfere with fonts that define the 200B character.
>
> Which begs the question whether a solution with an extra document class for
> this could not be the cleanest answer in the future. (But leave it as FFS,
> please)

My main goal is making export work better with defaults
(org-latex-default-class = "article"). We can, of course, change the
default class depending on document language, but that sounds rather
brittle. It will also make things less useful for people who, say,
prefer koma-letter classes.

So, \usepackage{ctex} sounds better compared to \documentclass{ctexart}

Anyway, back to the code review.

>    (defcustom org-latex-fontspec-config nil
>     ...
>      `:features': string or list of strings with font features.
>                   A potential fallback will be appended.
>    ...
>    (defcustom org-latex-fontspec-default-features nil
>      "List of default features for the fontspec package.
>    When nil, no default features are assumed.
>    When non-nil, the value should be an alist of (FEATURE . VALUE) that is
>    used to generate:

The way to define features is not the same for org-latex-fontspec-config
and org-latex-fontspec-default-features. Why not make
org-latex-fontspec-default-features support feature string?

>    (defcustom org-latex-polyglossia-font-config nil
>     `:font': a string with the system font name, mandatory
>     `:variant': a string for the font variant, (e.g. \"sf\", \"tt\", etc.)
>     `:tag': a string will substitute the language in the font definition.
>     `:props': a string for extra properties (e.g.\"Script=Hebrew\")

When reading the above, I barely noticed "mandatory". What about
something like

  `:font' (mandatory): ...

Same in `org-latex-fontspec-config'.

>    (if-let* ((lang-alist (assoc lang org-latex-language-alist))
>                               (lang-plist (cdr lang-alist)))
>                         (setq lang-tag (plist-get lang-plist :polyglossia)))

Nitpick: if-let* -> when-let*

>     defun org-latex--lualatex-babel-config (info)
>       "Return preamble components for babel on lualatex/xelatex.
>     INFO is the export communication channel.
>
>     Prefer #+LATEX_COMPILER: over `org-latex-compiler' and
>     and #+LANGUAGE over `org-export-default-language'.

The above paragraph is probably redundant.

>               do (let* ((props (alist-get bab-lang doc-babel-font-config nil 
> nil #'string=))
>                         (provide (or (plist-get props :provide) "import")))
>                    ;; \\babelprovide needs language and provide
>                    ;; it doesn't work on the default language
>                    (unless provide
>                      (setq provide "import"))

(unless provide ... is redundant because we have (or ... "import") just above.

>                                  (when (null font)
>                                    (error "Babel: font name missing script: 
> %s lang: %s" script lang))

It is not clear from this error which variable the user should fix.

>
>      ;; It there are font features, generate the declaration
>      (when current-default-features
>        (let ((def-feat-list
>               (cl-loop for (feat . val) in current-default-features
>                        collect (concat feat "=" val) into result
>                        finally return (mapconcat #'identity result ",\n  "))))
>          (insert (format "\\defaultfontfeatures{\n  %s\n}\n"
>                          def-feat-list))))

I think that this exact code appears twice, copy-pasted. Should it be a
separate helper function?

>    (defun org-latex-list-or-null-p (object)
>      "Return non-nil when `object' is a list or nil"
>      (or (null object)
>          (listp object)))

A simpler name like org-list-or-null-p would make more sense. It is a
very generic predicate, not really linked to latex.

> (defun org-latex--lualatex-fontspec-config (info)
> ...

See the attached diff with my suggestions. I had a really hard time
reading the code here and tried to rename the variables to make things
easier. With the diff, things are certainly easier to read for me, but
not sure about you. Please consider though.

Also, one test is failing on the latest branch.

diff --git a/lisp/ox-latex.el b/lisp/ox-latex.el
index afd3962f4..8f916b608 100644
--- a/lisp/ox-latex.el
+++ b/lisp/ox-latex.el
@@ -2192,12 +2192,12 @@ (defun org-latex--lualatex-fontspec-config (info)
           (insert (format "\\defaultfontfeatures{\n  %s\n}\n"
                           def-feat-list))))
       ;; add all fonts with fallback to fallback-alist
-      (dolist (fconfig current-fontspec-config)
-        (when-let* ((fname (car fconfig))
-                    (config-plist (cdr fconfig))
-                    (fallback (plist-get config-plist :fallback)))
-          (push (cons fname (concat "fallback_" fname)) fallback-alist)))
-      ;; (message "fallback-alist ==> %s" fallback-alist)
+      (cl-loop
+       for (font-family . config-plist) in current-fontspec-config
+       do
+       (when-let* ((fallback (plist-get config-plist :fallback)))
+         ;; (FONT-FAMILY . FALLBACK-NAME)
+         (push (cons font-family (concat "fallback_" font-family)) fallback-alist)))
       ;; As suggested by Jacob S. Gordon <[email protected]>
       ;; but with a twist
       ;; Remove the fallback list if we are not using lualatex
@@ -2208,59 +2208,54 @@ (defun org-latex--lualatex-fontspec-config (info)
           (warn "Fallback fonts only work with lualatex!")))
       (when fallback-alist ;; if there are fonts with fallbacks
         ;; create the directlua header
-        (dolist (fallback fallback-alist)
-          ;; (message "fallback ===> %s" fallback)
-          (when-let*
-              ((fbf-fname (car fallback))
-               (fbf-name (cdr fallback))
-               (fbf-plist (alist-get fbf-fname current-fontspec-config nil nil #'string=))
-               (fbf-flist (plist-get fbf-plist :fallback)))
-            ;; collect all falbacks for scripts that are present in the doc
-            (let ((fallback-flist
-                   (cl-loop for fpair in fbf-flist
-                            ;; check (car fpair) is in document scripts
-                            ;; and the fallback is not already in the result
-                            when (and (member-ignore-case (car fpair) doc-scripts)
-                                      (null (member-ignore-case (cdr fpair) fresult)))
-                            collect (cdr fpair) into fresult
-                            finally return fresult)))
-              ;; (message "fallback-flist ==> %s" fallback-flist)
-              (when fallback-flist
-                (unless directlua ;; add the heading before the first lua block
-                  (insert "\\directlua{\n")
-                  (setq directlua t))
-                ;; (setq fallback-flist (cl-remove-duplicates fallback-flist
-                ;;                                            :test #'equal))
-                (insert (format " luaotfload.add_fallback (\"%s\",{\n" fbf-name))
-                ;; Here we get the font fallbacks list
-                (dolist (fname fallback-flist)
-                  ;; If the string doesn't contain a ":"
-                  ;; it is assumed to be a font name that needs it at the end
-                  (unless (string-match-p ":" fname)
-                    (setq fname (concat fname ":")))
-                  ;; FIXME: when (car fpair) in document charsets
-                  (insert (format "  \"%s\",\n" fname)))
-                (insert " })\n")))))
+        (cl-loop
+         for (font-family . fallback-name) in fallback-alist
+         do
+         (when-let*
+             ((font-config (alist-get font-family current-fontspec-config nil nil #'string=))
+              (fallback (plist-get font-config :fallback)))
+           ;; collect all fallbacks for scripts that are present in the doc
+           (let ((fallback-fonts
+                  (cl-loop for (script . fallback-font) in fallback
+                           ;; check if SCRIPT is in document scripts
+                           ;; and the FALLBACK-FONT is not already in the result
+                           when (and (member-ignore-case script doc-scripts)
+                                     (null (member-ignore-case fallback-font result)))
+                           collect fallback-font into result
+                           finally return result)))
+             (when fallback-fonts
+               (unless directlua ;; add the heading before the first lua block
+                 (insert "\\directlua{\n")
+                 (setq directlua t))
+               ;; (setq fallback-fonts (cl-remove-duplicates fallback-fonts
+               ;;                                            :test #'equal))
+               (insert (format " luaotfload.add_fallback (\"%s\",{\n" fallback-name))
+               ;; Here we get the font fallbacks list
+               (dolist (fallback-font fallback-fonts)
+                 ;; If the string doesn't contain a ":"
+                 ;; it is assumed to be a font name that needs it at the end
+                 (unless (string-match-p ":" fallback-font)
+                   (setq fallback-font (concat fallback-font ":")))
+                 ;; FIXME: when script in document charsets
+                 (insert (format "  \"%s\",\n" fallback-font)))
+               (insert " })\n")))))
         (when directlua ;; if we have found any lua fallbacks, close the lua block
           (insert "}\n")))
-      ;; (message "fallbacks: %s" fallback-alist)
-      (dolist (fpair current-fontspec-config)
-        (when-let* ((ffamily (car fpair))
-                    (fplist  (cdr fpair))
-                    (ffont (plist-get fplist :font)))
-          (setq cjk-packages (or cjk-packages (string-match-p "^CJK" ffamily)))
-          (insert (format "\\set%sfont{%s}" ffamily ffont))
-          ;; add the extra features
-          (let ((ffeatures (plist-get fplist :features)))
-            (when (stringp ffeatures)
-              (setq ffeatures (list ffeatures))) ;; needs to be a list to concat a possible fallback
-            ;; (message "--> ffeatures: %s" ffeatures)
-            (when-let* ((fallback-fn (alist-get ffamily fallback-alist nil nil #'string=))
-                        (fallback-spec (and directlua (format "RawFeature={fallback=%s}" fallback-fn))))
-              (setq ffeatures (cl-concatenate #'list ffeatures (list fallback-spec))))
-            ;; (message "---> ffeatures %s" ffeatures)
-            (insert (org-latex--mk-options ffeatures)))
-          (insert "\n")))
+      (cl-loop
+       for (font-family . font-config) in current-fontspec-config
+       do
+       (when-let* ((font (plist-get font-config :font)))
+         (setq cjk-packages (or cjk-packages (string-match-p "^CJK" font-family)))
+         (insert (format "\\set%sfont{%s}" font-family font))
+         ;; add the extra features
+         (let ((features (plist-get font-config :features)))
+           (when (stringp features)
+             (setq features (list features))) ;; needs to be a list to concat a possible fallback
+           (when-let* ((fallback-name (alist-get font-family fallback-alist nil nil #'string=))
+                       (fallback-spec (and directlua (format "RawFeature={fallback=%s}" fallback-name))))
+             (setq features (cl-concatenate #'list features (list fallback-spec))))
+           (insert (org-latex--mk-options features)))
+         (insert "\n")))
       ;; If the CJK font families have been included
       ;; Check for polyglossia and/or babel and warn?
       ;; Or advise for these packages to be added to `org-latex-package-alist' ??
-- 
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