Re: [O] [patch] Snippet expansion

[Nicolas Goaziou]

Hello,

Rasmus <address@hidden> writes:

> The first patches adds string keys to snippet expansion.  For tempo, this
> is straight-forward.
>
> For the interactive prompt there’s an org-mks interface.  It limited to at
> most two keys (this shouldn’t be much of a limitation TBH).  So for
> instance if the key is "prop" the interactive prompt will be "p", "pr",
> "po" or "pp".
>
> The second patch are various improvements for org-tempo, e.g. to put the
> cursor at a better place.  org-tempo now also do some checks wrt. new
> structure templates.
>
> The third patch is more experimental and tries to be more "clever" when
> using the interactive interface, e.g. with newlines.  The code is now a
> lot more complicated, and I’m not sure it’s any more pleasant or
> DWIMish...

Thank you. Some comments follow (but none about usability).

>  (defcustom org-tempo-keywords-alist
> -  '((?L . "latex")
> -    (?H . "html")
> -    (?A . "ascii")
> -    (?i . "index"))
> +  '(("L" . "latex")
> +    ("H" . "html")
> +    ("A" . "ascii")
> +    ("i" . "index"))

You need to update :type keyword.

>  (defcustom org-structure-template-alist
> -  '((?a . "export ascii")
> -    (?c . "center")
> -    (?C . "comment")
> -    (?e . "example")
> -    (?E . "export")
> -    (?h . "export html")
> -    (?l . "export latex")
> -    (?q . "quote")
> -    (?s . "src")
> -    (?v . "verse"))
> +  '(("a" . "export ascii")
> +    ("c" . "center")
> +    ("C" . "comment")
> +    ("e" . "example")
> +    ("E" . "export")
> +    ("h" . "export html")
> +    ("l" . "export latex")
> +    ("q" . "quote")
> +    ("s" . "src")
> +    ("v" . "verse"))

Ditto.

> +(autoload 'org-mks "org-capture" "Select a member of an alist with multiple 
> keys." t)

If we're going to use "org-mks", I suggest to first move "org-mks" into
"org-macs.el" (which is the generic Org toolbox) first, then adapt
callers.

> +(defun org-insert-structure-template--mks ()

"--" is put after library prefix. Since that's org, it should be
"org--insert-structure-template-mks".

> +  "Present `org-structure-template-alist' with `org-mks'.
> +
> +- Menus are added if keys require more than one stroke.
> +- Tabs are added to single key entires when needing more than one stroke.
> +- Keys longer than two characters are reduced to two characters."
> +  (let* (case-fold-search
> +         (keys (mapcar 'car org-structure-template-alist))

Nitpick: 'car -> #'car

> +         (start-letters (delete-dups (mapcar (lambda (key) (substring key 0 
> 1)) keys)))
> +         (mks (mapcar (lambda (letter)
> +                        (list letter
> +                              (cl-remove-if-not
> +                            (apply-partially 'string-match-p (concat "^" 
> letter))

'string-match-p -> #'string-match-p

> +                               org-structure-template-alist :key 'car)))

Ditto.

> +                      start-letters)))
> +    (org-mks
> +     (apply 'append

Ditto.

> +            (mapcar (lambda (sublist)
> +                      (if (eq 1 (length (cadr sublist)))
> +                          (mapcar (lambda (elm)
> +                                    (list (substring (car elm) 0 1)
> +                                          (cdr elm) ""))
> +                                  (cadr sublist))
> +                        (let* ((topkey (car sublist))
> +                               (elms (cadr sublist))
> +                               (keys (mapcar 'car elms))
> +                               (longp (> (length elms) 3)))

Nitpick: longp -> long?

`longp' is not a predicate (i.e., a function).

> +                          (append
> +                           (list (list topkey
> +                                       (concat
> +                                     (mapconcat 'cdr

Guess what.

> +                                                (cl-subseq elms 0 (if longp 
> 3 (length elms)))
> +                                                ", ")
> +                                        (when longp ", ..."))))
> +                           (cl-mapcar 'list

Ahem.

> +(defun org-insert-structure-template--unique-keys (keys)
> +  "Make each key in KEYS unique and two characters long.
> +
> +For keys more than two characters, find the first unique
> +combination of the first letter and subsequent letters."
> +  (cl-loop for key in keys
> +           ;; There should at most be one key that is of length one.
> +           if (eq 1 (length key))
> +           collect (concat key "\t") into new-keys
> +           ;; All keys of two characters should be unique.
> +           else if (eq (length key) 2)
> +           collect key into new-keys
> +           else
> +           collect
> +           (cl-find-if-not (lambda (k) (member k new-keys))
> +                           (mapcar (apply-partially 'concat (substring key 0 
> 1))
> +                                   (split-string (substring key 1) "" t)))
> +           into new-keys
> +           finally return new-keys))

This looks overly complicated. Why not simply iterating over keys and
maintaining a list of keys stored so far, rejecting any duplicate along
the way?

> +(defun org-tempo-complete-tag ()
> +  (org-tempo--update-maybe)
> +  (call-interactively 'tempo-complete-tag))

*coughs*

> +      (indent (make-string col ? ))

"? " -> "?\s"

> +      (when region?
> +        (while (looking-at-p "^[ \t]*$")
> +          (delete-region (line-beginning-position)
> +                         (1+ (line-end-position)))))

(1+ (line-end-position)) -> (line-beginning-position 2)

The former can be greater than (point-max).

> +      (save-excursion
> +        (while (not (eobp))
> +          (unless (looking-at-p indent)
> +            (insert indent))
> +          (forward-line)))
> +      (insert
> +       indent
> +       (format "#+begin_%s%s\n" type (if specialp " " "")))

specialp -> special? (this is not a predicate).

Regards,

-- 
Nicolas Goaziou