From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [bug, patch, ox] INCLUDE and footnotes Date: Mon, 22 Dec 2014 12:10:15 +0100 Message-ID: <87wq5kvt2g.fsf@nicolasgoaziou.fr> References: <87h9x5hwso.fsf@gmx.us> <87oarcbppe.fsf@nicolasgoaziou.fr> <87fvcozfhf.fsf@gmx.us> <87h9x4bj33.fsf@nicolasgoaziou.fr> <87iohks4ne.fsf@gmx.us> <87d27rbvio.fsf@nicolasgoaziou.fr> <87bnnbhg2x.fsf@gmx.us> <878uifbjc7.fsf@nicolasgoaziou.fr> <87388j9qbv.fsf@gmx.us> <87y4q57t2i.fsf@nicolasgoaziou.fr> <87lhm4n9ky.fsf@pank.eu> <87mw6gyctg.fsf@nicolasgoaziou.fr> <87lhm01mj7.fsf@gmx.us> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:38423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y30rf-0000dp-H6 for emacs-orgmode@gnu.org; Mon, 22 Dec 2014 06:09:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y30rX-0006uz-E7 for emacs-orgmode@gnu.org; Mon, 22 Dec 2014 06:09:27 -0500 Received: from relay6-d.mail.gandi.net ([2001:4b98:c:538::198]:40075) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y30rX-0006ur-52 for emacs-orgmode@gnu.org; Mon, 22 Dec 2014 06:09:19 -0500 In-Reply-To: <87lhm01mj7.fsf@gmx.us> (rasmus@gmx.us's message of "Mon, 22 Dec 2014 02:49:32 +0100") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Rasmus Cc: emacs-orgmode@gnu.org Rasmus 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 =E2=8B=AF)? 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))) =20=20 > + (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,