Re: [PATCH 2/2] ox-texinfo: Define definition commands using description lists

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Thanks.

Jonas Bernoulli <jonas@bernoul.li> writes:

> +#+begin_example
> +- Key: C-n (do-something) ::
> +  This command does something.
> +
> +- User Option: do-something-somehow ::
> +  This option controls how exactly ~do-something~ does its thing.
> +#+end_example
> +
> +#+texinfo: @noindent
> +becomes
> +
> +#+begin_example
> +@table @asis
> +@item @kbd{C-c C-c} (@code{do-something})
> +@kindex C-c C-c
> +@findex do-something
> +This command does something.
> +@end table

There's a mismatch between the keys.

> +Key items don't have to specify the respective command in parenthesis
> +as done above; that part is optional.

Simply put:

Command in parenthesis, as done above, is optional.

> +Regardless of which approach you use, you must define the =kbd= macro
> +(see [[*Macro Replacement]]), which you can then use anywhere in the Org
> +file:
> +
> +#+begin_example
> +,#+macro: kbd (eval (let ((case-fold-search nil) (regexp (regexp-opt '("SPC" "RET" "LFD" "TAB" "BS" "ESC" "DELETE" "SHIFT" "Ctrl" "Meta" "Alt" "Cmd" "Super" "UP" "LEFT" "RIGHT" "DOWN") 'words))) (format "@@texinfo:@kbd{@@%s@@texinfo:}@@" (replace-regexp-in-string regexp "@@texinfo:@key{@@\\&@@texinfo:}@@" $1 t))))
>  #+end_example

Ouch. I don't think we should expect users to define this in order to
use the feature being implemented. IOW, it should work out of the box.

I think the functions responsible for generating the Texinfo code should
handle this without relying on the macro. Of course, if that part is
factored out, the macro might, in turn, make use of it.

> +(defconst org-texinfo--definition-command-regexp
> +  (format "\\`%s: \\(.+\\)"
> +          (regexp-opt
> +           (delq nil (mapcar #'cdr org-texinfo--definition-command-alist))
> +           1))

What is 1 meaning here? Do you mean t?


> +(defun org-texinfo--separate-definitions (tree _backend info)
> +  "Split up descriptive lists that contain Texinfo definition
> commands."

You need to document the arguments.
> +  (org-element-map tree 'plain-list
> +    (lambda (plain-list)
> +      (when (eq (org-element-property :type plain-list) 'descriptive)
> +        (let ((contents (org-element-contents plain-list))
> +              item items)

Nitpick: (items nil)

> +          (while (setq item (pop contents))

nitpick: Use dolist.

> +            (pcase-let ((`(,cmd . ,args) (org-texinfo--match-definition item)))
> +              (cond
> +               (cmd
> +                (when items
> +                  (org-texinfo--split-plain-list plain-list (nreverse items))
> +                  (setq items nil))
> +                (org-texinfo--split-definition plain-list item cmd args))
> +               (t
> +                (when args
> +                  (org-texinfo--massage-key-item plain-list item args))
> +                (push item items)))))
> +          (unless (org-element-contents plain-list)
> +            (org-element-extract-element plain-list)))))
> +    info)
> +  tree)
> +
> +(defun org-texinfo--match-definition (item)
> +  "Return a cons-cell if ITEM specifies a Texinfo definition command.
> +The car is the command and the cdr is its arguments."
> +  (let ((tag (car-safe (org-element-property :tag item))))
> +    (and tag
> +         (stringp tag)
> +         (string-match org-texinfo--definition-command-regexp tag)
> +         (pcase-let*
> +             ((cmd (car (rassoc (match-string-no-properties 1 tag)
> +                                 org-texinfo--definition-command-alist)))
> +              (`(,cmd ,category)
> +               (and cmd (save-match-data (split-string cmd " "))))
> +              (args (match-string-no-properties 2 tag)))
> +           (cons cmd (if category (concat category " " args) args))))))
> +
> +(defun org-texinfo--split-definition (plain-list item cmd args)
> +  "Insert a definition command before list PLAIN-LIST.
> +Replace list item ITEM with a special-block that inherits the
> +contents of ITEM and whose type and Texinfo attributes are
> +specified by CMD and ARGS."
> +  (let ((contents (org-element-contents item)))
> +    (org-element-insert-before
> +     (apply #'org-element-create 'special-block
> +            (list :type cmd
> +                  :attr_texinfo (list (format ":options %s" args))
> +                  :post-blank (if contents 1 0))
> +            (mapc #'org-element-extract-element contents))
> +     plain-list))
> +  (org-element-extract-element item))
> +
> +(defun org-texinfo--split-plain-list (plain-list items)
> +  "Insert a new plain list before the plain list PLAIN-LIST.
> +Remove ITEMS from PLAIN-LIST and use them as the contents of the
> +new plain list."
> +  (org-element-insert-before
> +   (apply #'org-element-create 'plain-list
> +          (list :type 'descriptive :post-blank 1)
> +          (mapc #'org-element-extract-element items))
> +   plain-list))
> +
> +(defun org-texinfo--massage-key-item (plain-list item args)
> +  "In PLAIN-LIST modify ITEM based on ARGS.
> +Reformat ITEM's tag property and add findex and kindex entries to
> +its content.  If the bullet is \"+\" then use \"@itemx\" and deal with
> +data from preceeding siblings that use such a bullet."
> +  (let (key cmd)
> +    (if (string-match " +(\\([^()]+\\)) *\\'" args)

Could you use `rx' here?

> +        (setq key (substring args 0 (match-beginning 0))
> +              cmd (match-string 1 args))
> +      (setq key args))
> +    (org-element-put-property
> +     item :tag
> +     (nconc (if (assoc "kbd" org-macro-templates)
> +                (let ((templates org-macro-templates))
> +                  (with-temp-buffer
> +                    (insert (format "{{{kbd(%s)}}}" key))

Here, there could be a function building the key chord, and you could
wrap the result into a raw string (see `org-export-raw-string').

Regards,
-- 
Nicolas Goaziou