Hi again
Long list, thanks. Went through it, item per item.
Current version in git reflects answers.
Comments inline...
On 1/11/25 14:23, Ihor Radchenko wrote:
> Pedro Andres Aranda Gutierrez <[email protected]> writes:
>> So, how do we proceed now?
> The next step is code review.
> Here are the initial comments on ox-latex.el:
> > +(defun string-or-null-p (object)
> > + "Return non-nil when `object' is a string or nil"
> > + (or (null object)
> > + (stringp object)))
>
> Please remove. This is a function from subr.el. I am not sure why you
> need to copy its definition.
Wasn't sure it was there and included it therefore.
Removed.
>> +(defcustom org-latex-multi-lang nil
>> + "The multi-lingual support package used by the LaTeX backend.
>> +
>> +Possible values are \"polyglossia\",\"babel\" , \"fontspec\" or `nil'.
>> +\"polyglossia\" or \"babel\" activates new implementations for
>> + either LaTeX package,
>> +`t' activates new font/multi-lingual support,
>> +`nil' means legacy support for babel/polyglossia."
>
>Please add reference to the in-buffer keyword. See how it is done in
>`org-latex-compiler' docstring.
Done
> Also, the text alignment is strange. On other docstrings as well -
> indentation of some lines seem arbitrary.
I hope I fixed it now.
> In addition, here and in other docstrings, please remove quotes from `t'
> and `nil'. By docstring convention, you should leave nil and t as is.
Done for ox-latex.el
>> (defcustom org-latex-fontspec-config nil
>> "This variable stores the configuration for the fontspec package.
>> By default, this variable is set to `nil' to generate no configuration.
>>
>> Each element is defined as
>> (`font-name' . `font-plist')
>> where `font-name' is the name in `\\set...font{}'
>> and `font-plist' is a plist. The keys for this plist are
>> `:font': system font name (mandatory)
>> `:features': string or list of strings with font features.
>> A potential fallback will be appended.
>> CAVEAT: features may be overwritten by fallback.
>> `:fallback': an alist of (`script' . `mapping') to map scripts in
the buffer
>> to their fallback font (optional).
>
> Having an example would be useful. You can refer to the manual if you do
> not want to duplicate examples here and there.
An example doesn't hurt... done
>> Place your customization in your Emacs initialisation or in
.dir-locals.el"
>
>I do not think that we need to explain this for a defcustom. It is a
>given, and we do not add such explanation to other custom options in Org.
OK. Done
> :type '(choice (const :tag "No template" nil)
> (alist :tag "fontspec config"))
I notice that you are using tabs for indentation. Our convention (and
settings in .dir-locals.el) disable indent-tabs-mode.
>> By defauly, this variable is set to `nil' to generate no configuration.
>> *default
> Also, `nil' -> nil
See above.
>> +(defcustom org-latex-babel-languages nil
>> + "A string with the babel languages.
>> +This is an alternative to adding the package in the LaTeX header"
>> + :group 'org-export-latex
>> + :package-version '(Org . "9.8")
>> + :type '(choice (const :tag "No babel languages" nil)
>> + (string :tag "babel language list"))
>> + :safe #'string-or-null-p)
>
>This option is not used anywhere.
Part of our discussions... commented out
> (defun org-latex--get-doc-scripts ()
> "This function gets the char-scripts used in the current buffer.
Please provide a proper docstring. See D.6 Tips for Documentation Strings
in Elisp manual.
>> (message "=> Scripts used in document: %s" scripts)
>
>Here and elsewhere, please get rid of the debugging messages. They are
>ok during testing, but not when we merge the branch upstream.
In this case it is not a debugging message. This message helps you,
for example, when you are defining the fallbacks. It is mentioned in
the manual.
>> (defun org-latex--mk-options (opts)
>> "Return empty string if OPTS is nil or a zero-length string.
>> If OPTS is a non-empty string, enclose it in square brackets.
>> If OPTS is a list of strings, trim each string in the list
>> before concatenating to a comma separated list and
>> returning the list enclosed in square brackets."
>
>The first line of the docstring should describe overall purpose/return
>value of the function. For example,
>
> Format OPTS as LaTeX option string [opt1,opt2,...] or empty string.
Done
>> (cond ((null opts) "")
>> ((stringp opts) (if (length= opts 0 ) "" (format "[%s]" opts)))
>> (t (format "[%s]" (mapconcat #'string-trim opts ","))))
>
>You can use `org-string-nw-p'
I'll keep it like that. The DOCSTRING was written to describe the logic and
I think it is clean enough. ()
>> "A minimal font code translator.
>> TODO: more languages and possible error conditions."
>
>Please provide a full docstring.
>Also, avoid TODO comments in the docstring. Instead, put them in the
>code, if you think they are necessary and mark them FIXME:, not
>TODO. TODO is way too common in org code as a keyword. FIXME is more
distinct.
Done.
> More generally, please review the docstrings to follow the Elisp
> conventions I linked to earlier.
In progress...
>> (concat "main="
>> ;; (mapconcat #'org-latex--pdflatex-ldf (split-string
langs "," t) ",")
>> (mapconcat #'org-latex--pdflatex-ldf langs ","))))
>
>Please clean up all irrelevant comments from the code.
This one done.
>> (let ((latex-langs
>> (or (plist-get info :languages) (list
org-export-default-language)))
>> (multi-lang
>> (or (plist-get info :latex-multi-lang)
org-latex-multi-lang))
>
>This should not be necessary. Export options should handle their
>default value just fine. You do not need to do double job that is
>supposed to by handled by ox.el API.
So let's trust ox.el :-)
>> (with-temp-buffer
>> (goto-char (point-min))
>> (when multi-lang
>> (insert "%% \\usepackage[utf8]{inputenc}\n")
>> (insert (format "\\usepackage%s{fontenc}\n"
(org-latex--fontenc-options latex-langs)))
>> (when (equal multi-lang "babel")
>> (insert (format "\n\\usepackage%s{babel}"
(org-latex--babel-ldf-list latex-langs)))))
>> (buffer-string))
>
>You can simply use `concat'. (concat "a" nil "b") => "ab"
You may, but this will be easier to remember (IMHO)
>> (compiler (or (plist-get info :compiler)
>> org-latex-compiler))
>
> (plist-get info :latex-compiler). Again, you do not need to manually
> check the default value. It is all defined in the :options-alist in the
> export backend definition.
You tell me once and I clean up (hopefully) everywhere. ;-)
>>
>> (unicode-math-options nil) ;; TODO
>
>What does this TODO mean?
I leave these options FFS
>> ;; These change inside `with-temp-buffer'
>> (fontspec-config org-latex-fontspec-config)
>
>You can actually do (org-latex-fontspec-config org-latex-fontspec-config)
>That will make it unnecessary to invent new variable names.
Let's keep this for a second iteration... It doesn't harm, does it?
>> (warn "polyglossia isn't supported by pdflatex!"))
>*Polyglossia
>Messages in Emacs start from capital letter.
Reworded: Polyglossia with capital P looks strange to me.
>> ;; Insert newline at the beginning to avoid too many empty lines
in the export
>> (cl-loop for lang in polyglossia-list do
>
> This comment does not seem relevant.
Made sense to me while coding...
> (let* ((lang-list (assoc lang org-latex-language-alist))
> (lang-plist (cdr lang-list))
Here, and in other places, see `alist-get'.
>> (insert (format "\n\\set%slanguage%s{%s}"
>> lang-type
>> (org-latex--mk-options lang-variant)
>> lang))
>> (setq lang-type "other")))
>
>I suggest moving let-binding of LANG-TYPE variable close to this part of
>the code. It is only used here anyway. AFAIR, you can probably define it
>right inside cl-loop (via let keyword).
I moved the binding to the beginning of the loop. The docs on cl-loop
are not
self-evident.
>> (defun org-latex--get-babel-lang (lang &optional default-lang)
>> (when (equal lang "AUTO")
>
>Please add missing docstrings.
Checking and adding where I find... recheck afterwards, please
>> (if-let* ((lang-alist (assoc lang org-latex-language-alist))
>> (lang-plist (cdr lang-alist)))
>> ;; (message "?? %s -> %s" lang lang-alist)
>> (let ((babel-lang (plist-get lang-plist :babel))
>> (babel-ini-only (plist-get lang-plist :babel-ini-only))
>> (babel-ini-alt (plist-get lang-plist :babel-ini-alt)))
>> (if babel-ini-alt babel-ini-alt
>> (or babel-lang babel-ini-only)))
>> lang))
>
>(let ((lang-plist (alist-get lang org-latex-language-alist nil nil
#'equal)))
> (or (plist-get lang-plist :babel-ini-alt)
> (plist-get lang-plist :babel)
> (plist-get lang-plist :babel-ini-only)
> lang))
Changed.
>> (defun org-latex--babel-langs-as-option(langs)
>
>This function is not used anywhere in the code.
Commented out. (2140)
>> "This function returns a string with the prelude part for
>> babel on lualatex/xelatex.
>
>You seem to be using a pattern "This function returns ..." in many
>places. Just say "Return ...". It is much shorter and is also Elisp
convention.
Replaced (hopefully) everywhere.
>> (let* ((compiler
>> (or (plist-get info :latex-compiler) org-latex-compiler))
>> (latex-babel-langs
>> (or (plist-get info :languages) (list
org-export-default-language)))
>
> Again, you do not need to check default value manually. ox API will do
> it for you.
Idem
>> ;; Tracing lost chars:
https://tex.stackexchange.com/questions/548901
>> ;; TODO: do we really need fontspec??
>> (insert "\\tracinglostchars=2\n%%\\usepackage{fontspec}")
>
> What is this TODO about?
Including fontspec seems sometimes redundant, but I'm still not 100% fine
with not including it because of tracinglostchars.
>> (insert (format"\n\\babelprovide[main,import]{%s}"
(org-latex--get-babel-lang (pop latex-babel-langs))))
>
>Please do not use pop. It will modify the list by side effect and might
>modify the actual customization by reference chain.
This is why I 'duplicate' variables in the let binding. Try this:
(setq test-list '("How" "do" "you" "do?"))
(let ((test-copy test-list))
(message "Inside let, before pop -> %s" test-copy)
(pop test-copy)
(message "Inside let, after pop -> %s" test-copy))
(message "Outside let -> %s" test-list)
> > (cl-loop for bab-lang in latex-babel-langs
>
> Using cl-loop here is an overkill. Just do
>
> (dolist (babel-lang latex-babel-langs) ...)
AFAIU it will translate into dolist internally. Since I'm also using it
for (cl-loop for (key . val) in alist ...
I was using it to make the function more uniform as for the number of
constructs it uses. It makes it easier to read (for me) and therefore
to program.
> Also, you seem to be shortening the variable names. IMHO, it is not
> desirable as it reduces readability. I prefer to have more readable
> (even if verbose) variable names in the code. This makes reading the
> code easier.
See above. Just being defensive, to avoid potential side effects.
IMvHO readability is marginally affected and the gain is worth it.
>> (let* ((props (alist-get bab-lang doc-babel-font-config nil nil
#'string=))
>> (provide (when props (plist-get props :provide))))
>> ;; \\babelprovide needs language and provide
>> ;; it doesn't work on the default language
>> (unless provide
>> (setq provide "import"))
>
> Can just
>
> (let* ((props (alist-get bab-lang doc-babel-font-config nil nil
#'string=))
> (provide (or (plist-get props :provide) "import"))) ...)
What would happen in `props' is nil?
>> "Returns the font prelude when we are on Lua/XeLaTeX and
>> we are using neither bale nor polyglossia"
>*babel
Thx.
>> (let ((compiler
>> (or (plist-get info :latex-compiler) org-latex-compiler))
>
> Again, you do not need to check default value manually.
Done (see above).
>> (cjk-packages nil) ;; will be need the packages to support
CJK fonts?
>*we need
Thx
>> ;;
>> ;; If we intend to use polyglossia, we can put it here.
>> ;; Same for babel on lua/xelatex
>> ;; They will automatically load, among others, fontspec
>> ;; If none are selected, then use fontspec directly
>> ;;
>> (insert "\\usepackage{fontspec}\n")
>> (insert (format "\\usepackage%s{unicode-math}\n"
>> (org-latex--mk-options unicode-math-options)))
Left-over from the devel process
>> ;; TODO: if we choose polyglossia,
>> ;; do we need fallbacks or
>> ;; should we warn if fallbacks are defined for polyglossia
>
>These comments make no sense to me.
Left-over. Thx
> ;; add all fonts with fallback to fallback-alist
That makes sense
>>Why are you talking about using polyglossia in
> (defun org-latex--lualatex-fontspec-config (info)
> "Returns the font prelude when we are on Lua/XeLaTeX and
> we are using neither bale nor polyglossia"
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
To stress that it is a fontspec-only. Anyhow, changed...
>> (when fallback-alist
>> (unless (equal compiler "lualatex")
>> (setq fallback-alist nil)
>> (warn "Fallback fonts only work with lualatex!")))
>
>AFAIU, this warning will be displayed every single time latex export is
>using xelatex, if there are fallback fonts in the fontspec-config. Is it
>really desired? Should be there an option to provide separate fontspec
>configuration to xelatex?
Well, I'd rather keep it. If someone doesn't fully understand this and
keeps trying to use xelatex with fallbacks, he should be warned each
single time.
>> (when-let* ((fallback-check fallback-alist) ;; don't do anything
when no fallbacks
>> (fallback-fn (alist-get ffamily fallback-alist nil nil
#'string=))
>
>The first line is redundant. (alist-get "foo" nil) will return nil and
>won't go into when-let* body.
Cool... wasn't my reading of the function...
>> ;; TODO: what about xpinyin??
>
>Why would you need xpinyin? It add latex commands to provide pinyin
>spelling of Chinese characters. AFAIU, it does not do anything
>automatically; you need to explicitly put a command. So, it does not
>seem necessary for automatic font configuration.
Not now. But why not let the possibility open and point to a
a possible place in the code to add it in the future?
>> "Return the font prelude for the current buffer as a string.
>
>You keep using "prelude". I think we refer to what we put before
>\begin{document} as "preamble" elsewhere in ox-latex.
Done the first time you found it. Might have been one of the days
where coding helped me cope with mankind's stupidity.
>> (let ((compiler
>> (or (plist-get info :latex-compiler) org-latex-compiler))
>> (multi-lang
>> (or (plist-get info :latex-multi-lang) org-latex-multi-lang)))
>
>Again, you do not need to check default value yourself.
see above ...
>> (defun keep-pkg (pkg use-driver)
>> "This function filters out the font management packages.
>> Keep the package if we are in legacy mode or
>> if it is not a font management package."
>> (or (null use-driver)
>> (not (member-ignore-case pkg '("fontenc" "fontspec" "inputenc"
"unicode-math")))))
>
>This function is (1) inappropriately named; (2) used exactly one.
>I think you can simply inline it.
(1) OK, renamed. (2) affects readability of the function below.
>> (defun org-latex--get-lang (lang)
>> "Return the long language name for LANG.
>> LANG can be a string, a comma-separated string or a list of languages.
>> When we have multiple languages, take the first."
>> (when lang
>> (if (stringp lang)
>> (setq lang (car (string-split lang "," t " ")))
>> (setq lang (car lang)))
>> (let ((plist (cdr (assoc lang org-latex-language-alist))))
>> ;; (message "%s" plist)
>> (or (plist-get plist :lang-name) lang))))
>
>Why do we need supporting comma-separated list here? In the manual, we
>explicitly say that the languages should be space-separated and then
>>ox.el handles splitting the value into a list. So, LANG should always be
a list.
Right. Left-over from LANGUAGE to LANGUAGES and back.
>> (multi-lang (or (plist-get info :latex-multi-lang)
>> org-latex-multi-lang)))
>
>Again, you do not need to use default value manually.
N/C... see above
>> ;;
>> ;; This is just a note for the integration of the new features
with main
>> ;;
>
>What does this comment mean?
Escaped cleanup.
