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

[Nicolas Goaziou]

Hello,

Rasmus <address@hidden> writes:

> Alan did some testing on a slightly older version of this patch and he
> managed to publish his book without errors and with working links.  So
> let's give it another shot.
>
> I briefly tested the output of LaTeX, html, texinfo, odt, md, and
> plaintext and made sure links work and that the right text is shown in
> the output.

Thanks. It looks mostly good. Some minor comments follow.

>> 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))
>
> But we are not checking that :CUSTOM_ID is unique.

This is not our problem, but user's.

> In ox-latex you're required to turn on a variable on to get this
> behavior (I could be mistaken here). For now I have done as you
> suggest. But I don't understand why we are favoring CUSTOM_ID here
> over the nice, unique label we've generated?

We could do the same as ox-latex, default to generated label, and
optionally allow users to use raw custom-id instead (with usual caveat).

Meanwhile, I think it is reasonable to stick to the current behaviour.

>> Last line:
>>
>>   (list headline (incf num))
>
> Oh incf is quite handy.  Didn't know that one.
>
> I leave it as (cons headline (list (incf num))).  Why?  'Cause that's
> the format used by `org-export--collect-headline-numbering'.  While
> simpler is nicer, I think it's better not to have to consider
> different data structures depending on whether data is from
> `org-export--collect-headline-numbering' or
> `org-export--collect-unnumbered-headline-id'.

I don't get your point. (cons 'a (list 'b)) is equivalent to (list 'a
'b). Why do you think this changes the data structure?

>>   (unless (org-some
>>            (lambda (h) (org-not-nil (org-element-property :UNNUMBERED h)))
>>            (org-export-get-genealogy headline))
>>     ...)
>
> Handy.  AFAIK BLOB is not a member of (org-export-get-genealogy BLOB)
> (or so the output suggests), so (or · ·) is still needed.

I think

  (org-some (lambda (h) ...)
            (cons headline (org-export-get-genealogy headline)))

is more elegant.

> +               ;; headline linked via CUSTOM_ID

  ;; Headline linked via CUSTOM_ID.

> +               (or (and (string= type "custom-id")
> +                        (org-element-property :CUSTOM_ID destination))
> +                   (org-export-get-headline-id destination info)
> +                   (t (error "Shouldn't reach here"))))
>                ;; What description to use?
>                (desc
>                 ;; Case 1: Headline is numbered and LINK has no
> @@ -3073,13 +3063,16 @@ holding contextual information."
>        (let* ((class-num (+ (org-export-get-relative-level parent info)
>                          (1- (plist-get info :html-toplevel-hlevel))))
>            (section-number
> -           (mapconcat
> -            'number-to-string
> -            (org-export-get-headline-number parent info) "-")))
> +           (and (org-export-numbered-headline-p parent info)
> +                (mapconcat
> +                 'number-to-string

Nitpick: #'number-to-string

> +          ;; Test if destination is a numbered headline

Missing full stop.

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

See above.

> +  (unless
> +      (or (org-element-property :UNNUMBERED headline)
> +       (org-some (lambda (head) (org-not-nil (org-element-property 
> :UNNUMBERED head)))
> +                 (org-export-get-genealogy headline)))
> +    (let ((sec-num (plist-get info :section-numbers))
> +       (level (org-export-get-relative-level headline info)))
> +      (if (wholenump sec-num) (<= level sec-num) sec-num))))

Per above

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

> +(ert-deftest test-org-export/org-export-get-headline-id ()
> +  "Test `org-export-get-headline-id' specifications."
> +  (should
> +   (equal "sec-1"
> +       (org-test-with-parsed-data "* Headline"
> +         (org-export-get-headline-id
> +          (org-element-map tree 'headline 'identity info t)
> +          info))))
> +  (should
> +   (equal "unnumbered-1"
> +       (org-test-with-parsed-data "* Headline\n:PROPERTIES:\n:UNNUMBERED: 
> t\n:END:"
> +         (org-export-get-headline-id
> +          (org-element-map tree 'headline 'identity info t)
> +          info))))
> +  (should
> +   (equal "unnumbered-1"
> +       (org-test-with-parsed-data "* Headline\n#+OPTIONS: num:nil"
> +         (org-export-get-headline-id
> +          (org-element-map tree 'headline 'identity info t)
> +          info)))))

I suggest to also test tricky inherited UNNUMBERED properties

  * H
  :PROPERTIES:
  :UNNUMBERED: t
  :END:
  ** H2
  :PROPERTIES:
  :UNNUMBERED: nil
  :END:
  *** H3


Regards,

-- 
Nicolas Goaziou