Re: [O] [patch, ox] Unnumbered headlines

[Nicolas Goaziou]

Hello,

Rasmus <address@hidden> writes:

> Another couple of small changes.

Thank you.

> Using this file:
>
>     * h1
>     :PROPERTIES:
>     :CUSTOM_ID: h1
>     :END:
>     ** h2
>     :PROPERTIES:
>     :unnumbered: t
>     :CUSTOM_ID: h2
>     :END:
>     *** h3
>     *** h4
>     * h5
>     :PROPERTIES:
>     :CUSTOM_ID: h5
>     :END:
>     [[*h1]] [[#h2]] [[*h4]] [[#h5]]
>     ** h6
>
> The output is now
>
>     \section{h1}
>     \label{sec-1}
>     \subsection*{h2}
>     \label{unnumbered-1}
>     \subsubsection*{h3}
>     \label{unnumbered-2}
>     \subsubsection*{h4}
>     \label{unnumbered-3}
>     \section{h5}
>     \label{sec-2}
>     \ref{sec-1} \hyperref[unnumbered-1]{h2} \hyperref[unnumbered-3]{h4} 
> \ref{sec-2}
>     \subsection{h6}
>     \label{sec-2-1}
>
> Which I think is quite good.

I agree.

> I don't know if the global unnumbered counter is made in the best way.
> I add another plist to info with the number.  This approach is cleaner
> than before since it's the numbering of unnumbered headlines is not in
> `org-export--collect-headline-numbering' which is complicated enough
> as it is.

14 locs long functions do not play in the "complicated enough" league.
Anyway, your implementation is fine, too.

> Should I write tests for the new behavior?  If so, tests for each
> backend or only for vanilla-ox functions?

Tests for "ox.el" are mandatory. See "test-ox.el"

> * ox.el (org-export--collect-headline-numbering): Return nil
> if unnumbered headline.

This is not exactly true: "Ignore unnumbered headlines." would be more
appropriate.

> (org-export-get-headline-id): New defun that returns a unique
> ID to a headline.

"New function." is enough.

> +           (if number
>               (if (atom number) (number-to-string number)
> -               (mapconcat 'number-to-string number "."))))))))
> +               (mapconcat 'number-to-string number "."))
> +             ;; unnumbered headline
> +             (when (eq 'headline (org-element-type destination))
> +               (format "[%s]" (org-export-data (org-element-property :title 
> destination) info)))))))))

While you're at it: #'number-to-string.

>             (ids (delq nil
>                        (list (org-element-property :CUSTOM_ID headline)
> -                            (concat "sec-" section-number)
> +                            (and section-number (concat "sec-" 
> section-number))
>                              (org-element-property :ID headline))))
> -           (preferred-id (car ids))
> +           (preferred-id (org-export-get-headline-id headline info))

I think the following is more in the spirit of the code (you don't
ignore :custom-id property):

  (ids (delq nil
             (list (org-element-property :CUSTOM_ID headline)
                   (org-export-get-headline-id headline info)
                   (org-element-property :ID headline))))
  (preferred-id (car ids))

>             (extra-ids (mapconcat
>                         (lambda (id)
>                           (org-html--anchor
> @@ -2807,21 +2807,7 @@ INFO is a plist holding contextual information.  See
>                       (org-element-property :raw-link link) info))))
>         ;; Link points to a headline.
>         (headline
> -        (let ((href
> -               ;; What href to use?
> -               (cond
> -                ;; Case 1: Headline is linked via it's CUSTOM_ID
> -                ;; property.  Use CUSTOM_ID.
> -                ((string= type "custom-id")
> -                 (org-element-property :CUSTOM_ID destination))
> -                ;; Case 2: Headline is linked via it's ID property
> -                ;; or through other means.  Use the default href.
> -                ((member type '("id" "fuzzy"))
> -                 (format "sec-%s"
> -                         (mapconcat 'number-to-string
> -                                    (org-export-get-headline-number
> -                                     destination info) "-")))
> -                (t (error "Shouldn't reach here"))))
> +        (let ((href (org-export-get-headline-id destination info))

This chuck needs to be updated since headline-id doesn't
replace :custom-id or :id properties.

>          (headline-label
> -         (let ((custom-label
> -                (and (plist-get info :latex-custom-id-labels)
> -                     (org-element-property :CUSTOM_ID headline))))
> -           (if custom-label (format "\\label{%s}\n" custom-label)
> -             (format "\\label{sec-%s}\n"
> -                     (mapconcat
> -                      #'number-to-string
> -                      (org-export-get-headline-number headline info)
> -                      "-")))))
> +         (format "\\label{%s}\n" (org-export-get-headline-id headline info)))

Ditto.

> -           (org-html--anchor
> -            (or (org-element-property :CUSTOM_ID headline)
> -                (concat "sec-"
> -                        (mapconcat 'number-to-string
> -                                   (org-export-get-headline-number
> -                                    headline info) "-")))
> +           (org-html--anchor (org-export-get-headline-id headline info)

Ditto.

>          (format "(%s)"
>                  (format (org-export-translate "See section %s" :html info)
> -                        (mapconcat 'number-to-string
> -                                   (org-export-get-headline-number
> -                                    destination info)
> -                                   ".")))))))
> +                        (if (org-export-numbered-headline-p destination info)
> +                            (mapconcat 'number-to-string
> +                                       (org-export-get-headline-number
> +                                        destination info)
> +                                       ".")
> +                          (org-export-get-alt-title headline info))))))))

No alt title please, as discussed before.

  (org-export-data (org-element-property :title headline) info)

>       ((org-export-inline-image-p link org-html-inline-image-rules)
>        (let ((path (let ((raw-path (org-element-property :path link)))
>                   (if (not (file-name-absolute-p raw-path)) raw-path
> @@ -354,9 +351,13 @@ a communication channel."
>       (if (org-string-nw-p contents) contents
>         (when destination
>           (let ((number (org-export-get-ordinal destination info)))
> -           (when number
> +           (if number
>               (if (atom number) (number-to-string number)
> -               (mapconcat 'number-to-string number "."))))))))
> +               (mapconcat 'number-to-string number "."))
> +             ;; unnumbered headline
> +             (and (eq 'headline (org-element-type destination))
> +               ;; BUG: shouldn't headlines have a form like [ref](name) in md
> +               (org-export-data (org-export-get-alt-title destination info) 
> info))))))))

Ditto. Also, #'number-to-string while you're at it.

>            (let* ((headline-no
> -                  (org-export-get-headline-number destination info))
> -                 (label
> -                  (format "sec-%s"
> -                          (mapconcat 'number-to-string headline-no "-"))))
> +                  (if (org-export-numbered-headline-p destination info)
> +                      (org-export-get-headline-number destination info)
> +                    (org-element-property :title destination)))

  (org-export-data (org-element-property :title destination) info)

However, I'm not sure this the expected headline-no.

> +     (unless (or (not (org-export-numbered-headline-p headline options))
> +                 (org-element-property :footnote-section-p headline))

  (when (and (not (org-element-type :footnote-section-p headline))
             (org-export-numbered-headline-p headline options))
    ...)

> +(defun org-export--collect-unnumbered-headline-id (data options)
> +  "Return a numbering of all unnumbered headlines.
> +
> +Unnumbered headlines are given numbered after occurrence."

You need to give details about the arguments, e.g.,

  "Associate a global counter to all unnumbered headlines
  DATA is the parse tree.  OPTIONS is a plist containing export options."

> +  (let ((num 0))
> +    (org-element-map data 'headline
> +     (lambda (headline)
> +       (unless (org-export-numbered-headline-p headline options)
> +         (cons headline (list (setq num (1+ num)))))))))

Last line:

  (list headline (incf num))

>  (defun org-export-get-headline-number (headline info)
> -  "Return HEADLINE numbering as a list of numbers.
> +  "Return numbered HEADLINE numbering as a list of numbers.
> +INFO is a plist holding contextual information."
> +  (and (org-export-numbered-headline-p headline info)
> +       (cdr (assoc headline (plist-get info :headline-numbering)))))

Use `assq' instead of `assoc'.

> +(defun org-export-get-unnumberd-headline-id (headline info)
> +  "Return unnumbered HEADLINE id as list of numbers.
>  INFO is a plist holding contextual information."
> -  (cdr (assoc headline (plist-get info :headline-numbering))))
> +  (and (not (org-export-numbered-headline-p headline info))
> +       (cdr (assoc headline (plist-get info :unnumbered-headline-id)))))

I don't think it is worth to make this function standalone. I don't see
any use case outside `org-export-get-headline-id'. I suggest to move it
there.

Also, `assoc' -> `assq'.

> +  (unless
> +      (or (org-export-get-node-property :UNNUMBERED headline)
> +       (loop for parent in (org-export-get-genealogy headline)
> +             when (org-export-get-node-property :UNNUMBERED parent)
> +             return t))

  (unless (org-some
           (lambda (h) (org-not-nil (org-element-property :UNNUMBERED h)))
           (org-export-get-genealogy headline))
    ...)


Regards,

-- 
Nicolas Goaziou