Re: [patch, ox-latex] captions and latex-environments

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Rasmus <rasmus@gmx.us> writes:

> The patch looks a bit dodgy, maybe because I used magit, which I don’t
> really understand, instead of the shell. I have attached it anew.

Thank you. Some comments follow.

> +(defun org-latex-environment--type (latex-environment)

It should be `org-latex--environment-type'.

> +  "Return the TYPE of LATEX-ENVIRONMENT.
> +
> +The TYPE is determined from the actual latex environment, and
> +could be a member of `org-latex-caption-above' or `math'."
> +  (let* ((value (org-remove-indentation
> +		 (org-element-property :value latex-environment)))
> +	 (latex-begin-re (cadr (assoc "begin" org-latex-regexps)))

I'd rather avoid using `org-latex-regexps', which predates the parser.
A hard-coded regexp is better.

> +	 (env (progn (string-match latex-begin-re value)
> +		     (match-string 2 value))))

Since environments do not necessary start with \begin{...}, I think the
following is better

  (and (string-match ...)
       (match-string ...))

> +    (cond
> +     ((string-match org-latex-math-environments-re value) 'math)
> +     ((string-match-p "tab\\(le\\|ular\\)" env) 'table)

This is a bit sloppy. In particular, it doesn't match all table
environments supported out of the box, e.g., "longtabu". Also, a list of
strings, compiler into a regexp with `regexp-opt' may be better.

>  	;; Environment is labeled: label must be within the environment
>  	;; (otherwise, a reference pointing to that element will count
> -	;; the section instead).
> +	;; the section instead). Also insert caption if `latex-environment'

Missing space after the full stop.

> +	;; is not a math environment.
>  	(with-temp-buffer
>  	  (insert value)
> -	  (goto-char (point-min))
> -	  (forward-line)
> -	  (insert (org-latex--label latex-environment info nil t))
> +	  (if caption-above-p
> +	      (progn
> +		(goto-char (point-min))
> +		(forward-line)
> +		(insert caption))
> +	    (goto-char (point-max))
> +	    (forward-line -1)
> +	    (insert caption))

Nitpick: you can move (insert caption) outside the (if ...) and
de-duplicate it.

Regards,

-- 
Nicolas Goaziou