Re: [PATCH] ox-latex: provide width and height options for images

[Nicolas Goaziou <n.goaziou@gmail.com>]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

Thank you for your patch. Here are a few comments.

> These are implemented with \resizebox, and thus are uniform across
> different types of image inclusion (\includegraphics, \input of tikz
> images).  This differs from the older way of using width and height
> optional args to \includegraphics.

I tend to agree with Rasmus. It would be better to keep height and width
options in \includegraphics when possible.

> Thus, the default value for org-latex-image-default-options is left
> untouched, to avoid breaking compatibility with older code.  After a
> transition period, the 0.9\linewidth value should be moved into
> org-latex-image-default-width, and the -options variable set to the
> empty string.

We don't need this precaution. The exporter code for 8.0 introduced many
incompatibilities already. Also, this one is easy to discover.

> +(defun org-not-nil-or-empty (v)
> +  "Return V if V is not nil, the string \"nil\", or a string
> +consisting of solely whitespace.  Otherwise return nil."
> +  (and (org-not-nil v)
> +       (org-string-nw-p v)
> +       v))

I'm not sure it's worth creating a new function for it. Anyway, the
first line of a docstring should be a sentence on its own.

>  (defcustom org-latex-image-default-option "width=.9\\linewidth"
>    "Default option for images."
>    :group 'org-export-latex
>    :type 'string)

We can set it to "".

> +(defcustom org-latex-image-default-width ""
> +  "Default option for images."
> +  :group 'org-export-latex
> +  :type 'string)
> +
> +(defcustom org-latex-image-default-height ""
> +  "Default option for images."
> +  :group 'org-export-latex
> +  :type 'string)

I think it's a good step forward. It will need to be documented in the
comments at the beginning of ox-latex.el, where all attributes
properties relative to different syntactical elements are explained.

>  (defcustom org-latex-default-figure-position "htb"
>    "Default position for latex figures."
>    :group 'org-export-latex
> @@ -1755,6 +1768,15 @@ used as a communication channel."
>  		   (format "[%s]" org-latex-default-figure-position))
>  		  (t ""))))
>  	 (comment-include (if (plist-get attr :comment-include) "%" ""))
> +	 ;; It is possible to specify width and height in the
> +	 ;; ATTR_LATEX line, and also via default variables.
> +	 (width (format "%s" (or (plist-get attr :width)
> +				 org-latex-image-default-width)))
> +	 (height (format "%s" (or (plist-get attr :height)
> +				  org-latex-image-default-height)))
> +	 (resize (format "\\resizebox{%s}{%s}{%%s}"
> +			 (if (org-not-nil-or-empty width) width "!")
> +			 (if (org-not-nil-or-empty height) height "!")))

Here, you can obtain \resizebox{!}{!}{%s}, which is wrong, isn't it?

>  	 ;; Options for "includegraphics" macro. Make sure it is
>  	 ;; a string with square brackets when non empty.  Default to
>  	 ;; `org-latex-image-default-option' when possible.
> @@ -1766,9 +1788,10 @@ used as a communication channel."
>  			  ((eq float 'float) "[width=0.7\\textwidth]")
>  			  ((eq float 'wrap) "[width=0.48\\textwidth]")
>  			  (t ""))))

This needs to be changed as these options would interfere with :width
argument. For example, (eq float 'float) could set :width property if it
is undefined. Obviously, this means the check has to be done before
WIDTH and HEIGHT strings are built.

> -	 (image-code (if (equal filetype "tikz")
> -			 (format "\\input{%s}" path)
> -		       (format "\\includegraphics%s{%s}" options path))))
> +	 (image-code (format resize
> +			     (if (equal filetype "tikz")
> +				 (format "\\input{%s}" path)
> +			       (format "\\includegraphics%s{%s}" options path)))))

See comments above.

Thank you again,


Regards,

-- 
Nicolas Goaziou