Re: Thoughts on Refactoring In-Buffer Completion In message.el

[Stefan Monnier <monnier@iro.umontreal.ca>]

Hi, thanks,

I think this is looking pretty good.
Here are my comments to your patch.

> @@ -8244,14 +8243,51 @@ message-email-recipient-header-regexp
>    :type 'regexp)
>  
>  (defcustom message-completion-alist
> -  `((,message-newgroups-header-regexp . ,#'message-expand-group)
> -    (,message-email-recipient-header-regexp . ,#'message-expand-name))
> -  "Alist of (RE . FUN).  Use FUN for completion on header lines matching RE.
> -FUN should be a function that obeys the same rules as those
> -of `completion-at-point-functions'."
> -  :version "27.1"
> +  `((,message-newgroups-header-regexp . (:category group
> +                                         :styles (substring partial-completion)
> +                                         :completions ,#'message-expand-group))
> +    (,message-email-recipient-header-regexp . (:category email
> +                                               :styles (substring partial-completion)
> +                                               :completions ,#'message-expand-name)))

`group` is too generic a name (remember that those category names are
"global" so they should be meaning in any other context than
message.el).
`newsgroup` maybe?



> +:completions
> +
> +    The function that provides completions, and that obeys the
> +    same rules as those of `completion-at-point-functions'.
> +    In-buffer completion will be performed as if
> +    `completion-at-point-functions' had been set to this value."

I think this should be a completion table, not a CAPF function.

> +          (_
> +           (let* ((recipe (alist-get message-email-recipient-header-regexp
> +                                     message-completion-alist))
> +                  (completions-function (plist-get recipe :completions)))
> +             (funcall completions-function))))))))

Hmm... `recipe` should be (car alist), rather than this
weird (alist-get ...), no?

And then we should do the (skip-chars-forw/backward "^, \t\n") dance
here, as well as the metadata dance to add the `category` if specified
by `recipe`.

> +;; set completion category defaults for newsgroup names based the on
> +;; settings in `message-completion-alist'
> +(let ((recipe (alist-get message-newgroups-header-regexp
> +                         message-completion-alist)))
> +  (pcase recipe
> +    ((pred functionp)
> +     (add-to-list 'completion-category-defaults
> +                  '(group (styles substring partial-completion))))
> +    (_
> +     (add-to-list 'completion-category-defaults
> +                  `(,(plist-get recipe :category)
> +                    (styles ,@(plist-get recipe :styles)))))))

I'd expect something like a `dolist` loop through
`message-completion-alist` rather than this weird (alist-get ...).
What am I missing?

Also, maybe rather than do this at top level, maybe we could add/set
`completion-category-defaults` directly from
`message-completion-function`, so it's done "more lazily" and so it
dynamically adapts to changes to `message-completion-alist`.

Tho, now that I think about it, having those styles in
`message-completion-alist` is weird: that var is a `defcustom`, hence
a user setting, yet we put it into `completion-category-defaults` which
is not meant to contain user settings (that's what
`completion-category-overrides` is for).

So maybe we should just hardcode

    (add-to-list 'completion-category-defaults
                 '(newsgroup (styles substring partial-completion))))
    (add-to-list 'completion-category-defaults
                 '(email (styles substring partial-completion))))

and remove the `:styles` from `message-completion-alist` since the user
should set `completion-category-overrides` instead.

[ It will also remove the problematic situation where
  `message-completion-alist` contains two entries with the same
  `:category` but with different `:styles`.  ]

> @@ -8402,7 +8474,12 @@ message--name-table
>          bbdb-responses)
>      (lambda (string pred action)
>        (pcase action
> -        ('metadata '(metadata (category . email)))
> +        ('metadata (let* ((recipe (alist-get message-email-recipient-header-regexp
> +                                             message-completion-alist))
> +                          (cat (plist-get recipe :category)))
> +                     (pcase recipe
> +                       ((pred functionp) '(metadata (category . email)))
> +                       (_ (when cat `(metadata (category . ,cat)))))))

I think we should do this metadata dance in
`message-completion-function` (where we already have the `recipe` at
hand).  Otherwise the `message-completion-alist` would be weird since
the `:category` entry would only be obeyed by those `:completions`
functions which happen to decide to obey it.


        Stefan