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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Rasmus <rasmus@gmx.us> writes:

> Attached is a patch that enables footnotes in INCLUDEd documents when
> using :lines and friends.  It stores the footnotes in a hash-table
> initialized in `org-export-expand-include-keyword' and updated via
> `org-export--prepare-file-contents'.  The footnotes are then inserted when
> all include keywords are expanded.

Thanks. Some more comments follow.

> At the moment only footnotes from INCLUDEs with :lines-like arguments will
> be picket up here.  But I think it might be nice to also use this
> functionality with footnotes when whole documents are included, and not
> include the footnote section directly from these documents.  Though I
> expect the to be accused of worm-nurturing, do consider this curious example:

[...]

>    2. fix the "bug" (IMO) that is that
>           #+INCLUDE: "/tmp/t00.org"
>           #+INCLUDE: "/tmp/t01.org"
>       Is "read" as
>           #+INCLUDE: "/tmp/t00.org" :minlevel N
>           #+INCLUDE: "/tmp/t01.org" :minlevel N+1
>       The easiest way I can think of would be to do a pre-scan of the
>       buffer to see if there exists any instances where include is only
>       separated by whitespace in which case they should have the same
>       level.

AFAICT, there's no reason to include a rule about whitespace separating
anything. Just make sure that any INCLUDE keyword that doesn't have
a :minlevel property gets one set to 1+N, where N is the current level
(or 0 if at top level).

Another option is to delay insertion of included files: expand them
completely in different strings, then replace keywords with appropriate
strings. IOW, just make sure expansion doesn't happen sequentially.

>  Objects can be extracted via =#+INCLUDE= using file links.  It is
> -possible to include only the contents of the object.  See manual for
> +possible to include only the contents of the object.  Further,
> +footnotes are now supported when using =#+INCLUDE=.  See manual for

This is not quite true. Footnotes are already supported with INCLUDE
keywords. This is the combination of :lines and footnotes that is new.
It is more a bugfix than a new feature.

> +	(footnotes (or footnotes (make-hash-table :test 'equal))))

Nitpick: (make-hash-table :test #'equal)

> +		   (goto-char (point-min))
> +		   (while (and (search-forward-regexp org-footnote-re nil t))
> +		     (let* ((reference (org-element-context))
> +			    (type (org-element-type reference))
> +			    (label (org-element-property :label reference)))
> +		       (when (and label (eq type 'footnote-reference))
> +			 (unless (org-footnote-get-definition label)
> +			   (save-excursion
> +			     (org-footnote-create-definition label)
> +			     ;; We do not need an error here since ox
> +			     ;; will complain if a footnote is missing.
> +			     (insert (or (gethash label footnotes) "")))))))

Why is the above necessary? Shouldn't you only insert footnotes
definitions at the end of the master document (i.e. when INCLUDED is
nil)? I think a `maphash' is enough.

Also, looking for every footnote reference sounds tedious. You should
simply insert every footnote definition collected there, and filter out
unnecessary definitions at another level (e.g., before storing it in the
hash table).

> +      (when id
> +	(unless (eq major-mode 'org-mode)
> +	  (let ((org-inhibit-startup t)) (org-mode)))

Is it necessary?

> +	(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 (and (eq type 'footnote-reference))
                   ^^^
Typo.

> +	      (goto-char (org-element-property :begin reference))
> +	      (when label
> +		(goto-char (org-element-property :begin reference))

You are already at reference beginning.

> +		(forward-char 4)
> +		(insert (format "%d-" id))
> +		(and (not (eq footnote-type 'inline))
> +		     (let ((new-label (org-element-property
> +				       :label (org-element-context))))

Why do you need to parse the new label, since you know it already:

  (concat (format "%d-" id) label)

> +		       (save-restriction
> +			 (save-excursion
> +			   (widen)

`save-restriction' + `save-excursion' + `widen' = `org-with-wide-buffer'

> +			   (org-footnote-goto-definition label)
> +			   (let ((definition (org-element-context)))
> +			     (and include-footnotes

Nitpick:

  (when include-footnotes ...

> +				  (puthash new-label
> +					   (org-element-normalize-string
> +					    (buffer-substring
> +					     (org-element-property
> +					      :contents-begin definition)
> +					     (org-element-property
> +					      :contents-end definition)))
> +					   footnotes))

Here you could check if :contents-begin is within LINES, in which case
the definition needs not be inserted at the end of the master document.

Regards,

-- 
Nicolas Goaziou