Re: [O] [patch] Improved block insertion

[Nicolas Goaziou]

Hello,

Rasmus <address@hidden> writes:

> These patches improve the block insertion mechanisms using both the
> keyboard binding and org-tempo.

Thank you. Some quick comments follow.

> Patch 6 changes the key binding of blocks to "C-c C-," as discussed in
> December 2017.  Let me know if this key is OK and if the old key should
> still be kept.

I don't think the old key-binding should be kept.

> +(defun org--mks-read-key (allowed-keys prompt)
> +  "Read a key and ensure it is a member of ALLOWED-KEYS.
> +
> +Tab, space and RET are treated equivalently."

Nitpick: No need for a blank line in the docstring.

Also: TAB, SPC and RET are ...

> +  (let* ((char (read-char-exclusive prompt))
> +      (key (char-to-string
> +            (cond ((memq char '(?\s ?\t ?\r)) ?\t)
> +                  (t char)))))

Suggestion:

  (key (pcase (read-char-exclusive prompt)
        ((or ?\s ?\t ?\r) ?\t)
        (char char)))

> +    (let ((menu-choice (org--insert-structure-template-mks)))
> +      (if (equal (nth 0 menu-choice) "\t")
> +       (read-string "Structure type: ")
> +     (nth 1 menu-choice)))))

(pcase (org--insert-structure-template-mks)
  (`("\t" . ,_) (read-string "Structure type: "))
  (`(,_ ,choice . ,_) choice))
  
>    (let* ((region? (use-region-p))
>        (s (if region? (region-beginning) (point)))
>        (e (copy-marker (if region? (region-end) (point)) t))
> @@ -13568,7 +13652,7 @@ headlines matching this string."
>         ;; compile tags for current headline
>         (setq tags-list
>               (if org-use-tag-inheritance
> -                 (apply 'append (mapcar 'cdr (reverse tags-alist)))
> +                 (apply #'append (mapcar 'cdr (reverse tags-alist)))

Nitpick: (mapcar #'cdr ...)

> +(defun org-tempo--keys ()
> +  (mapcar (lambda (pair) (format "<%s" (car pair)))
> +       (append org-structure-template-alist
> +               org-tempo-keywords-alist)))

Missing docstring.

> +(defun org-tempo--update-maybe ()
> +  "Test if new tags have been added."

I think you should clarify the docstring. It doesn't only test if new
tags have been added, but it also add templates.

Regards,

-- 
Nicolas Goaziou