Re: [bug, patch, ox] INCLUDE and footnotes

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Rasmus <rasmus@gmx.us> writes:

> Thanks for the notes.  Hopefully patch one if good now.

Thanks. Some comments follow.

> * ox.el (org-export--prepare-file-contents): Preserve footnotes
> when using the LINES argument.  New optional argument FOOTNOTES.
>  (org-export-expand-include-keyword): New optional argument
>  FOOTNOTES.
> * test-ox.el: Add test for INCLUDE with :lines and footnotes.

Please name the test modified.

> +	(include-re "^[ \t]*#\\+INCLUDE:"))

Why do you need it since you only use this regexp once in the function?

> +	       ;; Expand footnotes after all files have been
> +	       ;; included.  Footnotes are stored in an
> +	       ;; `org-footnote-section' if that variable is
> +	       ;; non-nil.  Otherwise they are stored close to the definition.

Don't bother about `org-footnote-section'. Even if the variable is
non-nil, footnote definitions are allowed everywhere. So in any case,
the very end of the master document is an appropriate location.

Also, inserting definitions close to the reference is wrong, as
explained in this thread (it could break structure around reference,
e.g., a plain list).

> +	      (when (and (not included) (> (hash-table-count footnotes) 0))
> +		(org-with-wide-buffer
> +		 (goto-char (point-min))
> +		 (if org-footnote-section
> +		     (progn
> +		       (or (search-forward-regexp
> +			    (concat "^\\*[ \t]+"
> +				    (regexp-quote org-footnote-section)
> +				    "[ \t]*$")
> +			    nil t)
> +			   (and
> +			    (goto-char (point-max))
> +			    (insert (format "* %s" org-footnote-section))))
> +		       (insert "\n")
> +		       (maphash (lambda (ref def)
> +				  (insert (format "[%s] %s" ref def) "\n"))
> +				footnotes))
> +		   ;; `org-footnote-section' is nil.  Insert definitions close to references.
> +		   (maphash (lambda (ref def)
> +			      (save-excursion
> +				(search-forward (format "[%s]" ref))
> +				(org-footnote-create-definition ref)
> +				(insert def "\n")))
> +			    footnotes))))))))))))

IOW, I think

  (org-with-wide-buffer
   (goto-char (point-max))
   (unless (bolp) (insert "\n"))
   (maphash (lambda (label definition) (insert "[" label "] " definition "\n"))
            footnotes))))))

is enough.

> +      (let* ((lines (and lines (split-string lines "-")))
> +	     (lbeg (and lines (string-to-number (car lines))))
> +	     (lend (and lines (string-to-number (cadr lines)))))

This is not needed. `point-min' and `point-max' refer to the boundaries
of the area to be included. It avoids relying on the expensive
`line-number-at-pos' function later.

Moreover, I suggest store (point-max) as a marker since you are going to
modify contents of the buffer (e.g., adding ID to labels).

> +	(goto-char (point-min))
> +	(while (re-search-forward org-footnote-re nil t)
> +	  (let* ((reference (org-element-context))
> +		 (type (org-element-type reference))
> +		 (footnote-type (org-element-property :type reference))
> +		 (label (org-element-property :label reference)))
> +	    (when (eq type 'footnote-reference)

The order is confusing here. First you check if you're really at
a footnote reference, then you bind LABEL and FOOTNOTE-TYPE. Actually,
the latter doesn't even need to bound since you use it only once.

> +	      (goto-char (org-element-property :begin reference))
> +	      (when label
> +		(forward-char 4)
> +		(insert (format "%d-" id))

This looks wrong. Labels can be "1", "fn:label" or even "fn:" in
anonymous footnotes. You need to check if label matches "\\`[0-9]+\\'",
in which case prepending "fn:" is also necessary.

> +		(and (not (eq footnote-type 'inline))

   (unless (eq footnote-type 'inline) ...)

or

  (when (eq (org-element-property :type reference) 'standard) ...)

> +		     (org-with-wide-buffer
> +		      (org-footnote-goto-definition label)
> +		      (beginning-of-line)
> +		      (org-skip-whitespace)

Footnote definitions start at column 0. Skipping white spaces is not
needed.

> +		      (forward-char 4)
> +		      (insert (format "%d-" id))

Dangerous. See above.

> +		      (let ((definition (org-element-context)))
> +			(when (and lines
> +				   (or (< lend (line-number-at-pos
> +						(org-element-property
> +						 :contents-begin definition)))
> +				       (> lbeg (line-number-at-pos
> +						(org-element-property
> +						 :contents-begin definition)))))
> +			  (puthash (org-element-property :label definition)
> +				   (org-element-normalize-string
> +				    (buffer-substring
> +				     (org-element-property
> +				      :contents-begin definition)
> +				     (org-element-property
> +				      :contents-end definition)))
> +				   footnotes)))))))))))

You don't need this part. Basically move to the definition of the
current reference in a wide buffer. If you're not within narrowed
boundaries stored before, put it in the hash table. Otherwise, skip it.
It doesn't require `org-element-context' or `line-number-at-pos'.


Regards,