Ihor Radchenko <[email protected]> writes:
> Continuing code review.
>
> One general comment - all the changes are to be marked as Org 10.0, not
> 9.7/9.8
I will do this and the if-let -> if-let* changes at the very end, as
these changes will be spread across the whole file(s) and will make
rebasing more difficult in the meantime.
> Also, please address all the compiler warnings.
I'm addressing them as I find them. If any remain after all the code
reviews I'll do one more pass.
org-compat changes:
> In org-compat, we obsolete the old functions that create latex
> previews. However, due to changes in the customization
> (`org-format-latex-options') that no longer contain :html-background,
> the obsolete commands will be broken. You need to adjust these obsolete
> functions to keep working until we remove them.
Changed org-format-latex-options documentation.
I don't know how to adjust ox-html so that :html-background (etc) in
org-format-latex-options continues to be respected.
I can modify org-html-prepare-latex-images so that
org-format-latex-options is respected:
(let ((obsolete-image-options
;; FIXME: Fix for `org-format-latex-options' specification
;; Delete after removing this obsolete option
`(,@(and-let* ((background (plist-get
org-latex-preview-appearance-options
:html-background)))
(list :background background))
,@(and-let* ((foreground (plist-get
org-latex-preview-appearance-options
:html-foreground)))
(list :foreground foreground))
,@(and-let* ((scale (plist-get org-latex-preview-appearance-options
:html-scale)))
(list :scale scale)))))
(apply #'org-latex-preview-cache-images parse-tree info
;; Do not copy preview images to :image-dir if
;; inlining of images in html is requested
(org-combine-plists
image-options
obsolete-image-options
(and (memq image-type inline-condition)
(list :image-dir nil)))))
But note that image-options is obtained as (plist-get info
:html-latex-image-options), so the values in org-format-latex-options
will override whatever is specified in an exporter derived from the html
exporter will for the :html-latex-image-options key. I think this is
the correct behavior, but I want to check.
>> (define-obsolete-variable-alias
>> 'org-format-latex-header 'org-latex-preview-preamble "9.7")
>
> This is not just obsoletion. The value have been changed.
> We need to keep the old value, singling out the old variable.
> Otherwise, the obsolete `org-create-formula-image' will stop working.
Done, I've restored the variable's original value and marked it
obsolete.
> What will happen if someone uses the old default value with the new
> `org-latex-preview-preamble'?
It will cause all kinds of sizing errors. This shouldn't happen now
that the two are not aliases.
>> (define-obsolete-variable-alias
>> 'org-preview-latex-default-process 'org-latex-preview-process-default
>> "9.7")
>
> Will the entries from org-latex-preview-process-alist work with
> `org-create-formula-image'? I think they will not. In particular
> old ("dvipng -D %D -T tight -o %O %f") is not compatible with
> ("dvipng --follow -D %D -T tight -bg Transparent --depth --height -o
> %B-%%09d.png %f")
> I think we should use %O placeholder to pass the correct file name
> rather than forcing "%B-%%09d.png".
%O will not work, we generate multiple images in one LaTeX run and so we
need the -%%09d field to specify the numbering to dvipng.
The LaTeX and dvipng process filters then match up the input LaTeX
fragments (numbered internally the same way) with the output PNG files.
Explicit numbering is required because some images will fail to be
generated, and we will get off-by-one and other index errors when
matching the input fragment ↔ output image otherwise.
What should I do here about the compatibility? It seems like it should
be okay to break this.
>> (define-obsolete-function-alias
>> 'org-clear-latex-preview 'org-latex-preview-clear-overlays "9.7")
>
> We should then also get rid of
> (define-obsolete-function-alias 'org-remove-latex-fragment-image-overlays
> 'org-clear-latex-preview "9.3")
> That's now super-obsolete.
Removed.
>> (define-obsolete-function-alias
>> 'org--get-display-dpi 'org-latex-preview--get-display-dpi "9.7")
>
> This is redundant. There is no need to obsolete internal functions.
I believe I kept this in because some external packages (like PDF tools)
were using it. But I have removed it now.
>> (define-obsolete-variable-alias
>> 'org-latex-to-mathml-jar-file 'org-mathml-converter-jar-file "9.7")
>> (define-obsolete-variable-alias
>> 'org-latex-to-mathml-convert-command 'org-mathml-convert-command "9.7")
>> (define-obsolete-function-alias
>> 'org-create-math-formula 'org-mathml-convert-latex "9.7")
>
> There are still mentions to the obsolete names in the code and in the
> manual.
I didn't even realize ox-mathml was part of this patchset. I think
Timothy added this at some point -- this is the first time I'm looking
at it.
I've fixed the mentions of the old names except in org-compat.
>> (define-obsolete-function-alias
>> 'org-latex-mathml-directory 'org-mathml-export-directory "9.8")
>
> Obsoletion not announced in the news.
> Also, org-mathml-export-directory is ignored in the new code.
I haven't fixed this. I'm not familiar with this code, not sure what
the idea is here. I'll ask Timothy.
>> ;; (eval-after-load 'ox-odt '(ad-deactivate 'org-format-latex-as-mathml))
>
> There is a mention of now-obsolete `org-format-latex-as-mathml'. We can
> probably remove that comment. We may also review why that comment is
> there and possibly fix the issue (optional).
I've removed the comment.
>> (make-obsolete-variable
>> 'org-preview-latex-image-directory 'org-latex-preview-cache "9.7")
>
> The new default value is 'persist, which will break obsolete functions.
Changed to
(make-obsolete-variable
'org-preview-latex-image-directory
"Use `org-latex-preview-cache' instead."
"10.0")
IIUC, now there won't be a clash with the default value of
org-latex-preview-cache.
Karthik
