Hello, Aaron Ecay <aarone...@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