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.
> +(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.
Also, the text alignment is strange. On other docstrings as well -
indentation of some lines seem arbitrary.
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.
> (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.
> 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.
> :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
> +(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.
> (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.
> (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.
> (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'
> "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.
More generally, please review the docstrings to follow the Elisp
conventions I linked to earlier.
> (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.
> (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.
> (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"
> (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.
> (unicode-math-options nil) ;; TODO
What does this TODO mean?
> ;; 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.
> (warn "polyglossia isn't supported by pdflatex!"))
*Polyglossia
Messages in Emacs start from capital letter.
> ;; 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.
> (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).
> (defun org-latex--get-babel-lang (lang &optional default-lang)
> (when (equal lang "AUTO")
Please add missing docstrings.
> (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))
> (defun org-latex--babel-langs-as-option(langs)
This function is not used anywhere in the code.
> "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.
> (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.
> ;; 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?
> (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.
> (cl-loop for bab-lang in latex-babel-langs
Using cl-loop here is an overkill. Just do
(dolist (babel-lang latex-babel-langs) ...)
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.
> (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"))) ...)
> "Returns the font prelude when we are on Lua/XeLaTeX and
> we are using neither bale nor polyglossia"
*babel
> (let ((compiler
> (or (plist-get info :latex-compiler) org-latex-compiler))
Again, you do not need to check default value manually.
> (cjk-packages nil) ;; will be need the packages to support CJK fonts?
*we need
> ;;
> ;; 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)))
> ;; TODO: if we choose polyglossia,
> ;; do we need fallbacks or
> ;; should we warn if fallbacks are defined for polyglossia
> ;; add all fonts with fallback to fallback-alist
These comments make no sense to me.
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"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> (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?
> (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.
> ;; 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.
> "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.
> (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.
> (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.
> (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.
> (multi-lang (or (plist-get info :latex-multi-lang)
> org-latex-multi-lang)))
Again, you do not need to use default value manually.
> ;;
> ;; This is just a note for the integration of the new features with main
> ;;
What does this comment mean?
--
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>
