Karthik Chikmagalur <[email protected]> writes:
> I'm working on org-latex-preview actively this week, so even incremental
> feedback will be helpful as I can get started on it.
Now, ox-odt.el
> (defcustom org-odt-latex-image-options
> '(:foreground "Black" :background "Transparent"
> :page-width 1.0 :scale 1.0 :inline nil)
:inline does nothing here and is not documented.
> - (cl-assert (equal "file" (org-element-property :type element)))
> - (let* ((src (let ((raw-path (org-element-property :path element)))
> - (cond ((file-name-absolute-p raw-path)
> - (expand-file-name raw-path))
> - (t raw-path))))
> + (let* ((src (let* ((type (org-element-property :type element))
> + (raw-path (org-element-property :path element)))
> + (if (file-name-absolute-p raw-path) raw-path
> + (expand-file-name raw-path))))
This change makes no sense to me.
1. Why removing cl-assert? Are non-file links supported now?
2. The new code is
(src (let* ((type (org-element-property :type element))
(raw-path (org-element-property :path element)))
(if (file-name-absolute-p raw-path) raw-path
(expand-file-name raw-path))))
(src-expanded (if (file-name-absolute-p src) src
(expand-file-name src (file-name-directory
(plist-get info :input-file)))))
So, you effectively removed the else branch in src-expanded. This is not right.
> (--em-to-cm
> ;; FIXME: Hardcoded default font-size to 12 according to the
> ;; default value of styles.xml in org-odt-styles-dir. I
> ;; don't know how how to determine this dynamically.
> (lambda (size) (and size (* 12 0.0352778 size))))
> ...
> (size (org-odt--image-size
> src-expanded info width height
> (let ((scale (plist-get attr-plist :scale)))
> (and scale (read scale)))
> nil ; embed-as
> "paragraph" ; FIXME
> ))
Why can't you use org-odt--image-size as is?
> (link (list 'link
> (list :type "file"
> :path path
> :format 'bracket
> :raw-link (format "file:%s" path))))
Please use org-element-create. Same later.
> (if-let ((latex-frag (org-element-property :value latex))
> ... (replacement ...))
> (org-element-put-property replacement :replaces
> (list (org-element-type latex)
> (list :value latex-frag)))
It is not guaranteed that replacement is non-nil in the else clause of
if-let. Looks fishy.
Also, you have a lot of code duplication here. Why is it necessary? Can
it be avoided?
--
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>
