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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Rasmus <rasmus@gmx.us> writes:

> Thanks for the comments.  I managed to make the patch less
> complicated.

Thanks. Another round of comments follows.

> I did not know markers but they seem perfect in this case.  The manual
> mentions setting markers to nil after use.  I guess it's not necessary
> here since they are in a (let ⋯)?

It is. Binding between the symbol and the marker disappears with the
`let', but the marker still exists. It cannot be GC'ed unless it is set
to nil.

It is not terribly important here as we're working in a temporary buffer
anyway, so the marker will ultimately disappear at the end of the export
process. However, it's a good habit to have.

>>> +	(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.
>
> I check if I'm at a footnote reference 'cause I never want to deal with a
> footnote-*definition* directly.  AFAIK org-footnote-re matches both.  It
> seems a footnote-reference can never be at the beginning of line, but I
> would still prefer a more explicit test through org-element.

I wasn't clear. The check is important. The minor issue is that you bind
LABEL and FOOTNOTE-TYPE too early, before making sure you are at
a footnote reference. It would be more logical to do

  (let ((object (org-element-context)))
    (when (eq (org-element-type object) 'footnote-reference)
      (let ((footnote-type (org-element-property :type object))
            (label (org-element-property :label object)))
        ...)))

> The only "bug" *I'm aware of* is that it will pick up the wrong new-label
> for footnote for something like [fn:ref with space].  But this is anyway
> not a proper label, I think.  Is that OK?

[fn: ref with space]

> * test-ox.el (test-org-export/expand-include): Add test for INCLUDE with :lines and footnotes.

Line is too long.

> +	      (when (and (not included) (> (hash-table-count footnotes) 0))
> +		(org-with-wide-buffer
> +		 (goto-char (point-max))
> +		 (unless (bolp) (insert "\n"))
> +		 (maphash (lambda (ref def) (insert (format "[%s] %s\n" ref def)))
> +			  footnotes)))))))))))

  (unless included ...)

is sufficient. No need to check for table emptiness (although it doesn't
cost much): maphash will simply do nothing. If

  (unless (bolp) (insert "\n"))

bothers you, you can drop it and use

  (lambda (ref def) (insert (format "\n[%s] %s\n" ref def)))

in the maphash instead.

> +      (let ((marker-min (point-min-marker))

This marker is not necessary since you're not going to add contents
before (point-min) anyway. A plain number is enough.

> +	    (marker-max (point-max-marker)))

As explained above, don't forget

  (set-marker marker-max nil)

at the end of the `let'.

> +	(goto-char (point-min))
> +	(while (re-search-forward org-footnote-re nil t)
> +	  (let* ((reference (org-element-context))
> +		 (label (org-element-property :label reference))
> +		 (digit-label (and label (org-string-match-p "\\`[0-9]+\\'" label))))
> +	    (when (eq (org-element-type reference) 'footnote-reference)

See above about order of bindings.

> +	      (goto-char (1+ (org-element-property :begin reference)))
> +	      (when label
> +		(let ((new-label
> +		       (buffer-substring-no-properties
> +			(point)
> +			(progn (if digit-label (insert (format "fn:%d-" id))
> +				 (forward-char 3)
> +				 (insert (format "%d-" id)))
> +			       (1- (search-forward "]"))))))

> +		  (unless (eq (org-element-property :type reference) 'inline)

A comment about what we're going to do would be nice.

> +		    (org-with-wide-buffer
> +		     (let* ((definition (org-footnote-get-definition label))
> +			    (beginning (set-marker (make-marker) (nth 1 definition))))

Nitpick:

  (beginning (copy-marker (nth 1 definition)))
  
> +		       (goto-char (1+ beginning))
> +		       (if digit-label (insert (format "fn:%d-" id))
> +			 (forward-char 3)
> +			 (insert (format "%d-" id)))
> +		       (when (or (< beginning marker-min) (> beginning marker-max))
> +			 (puthash new-label (nth 3 definition)
> +				  footnotes))))))))))))

  (set-marker beginning nil)


Regards,