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

[Stefan Monnier]

> +When RECIPE is a function, it is called for completion.  RECIPE
> +should be a function that obeys the same rules as those of
> +`completion-at-point-functions'.

Right.

> +When RECIPE is a plist, the stored properties are used to control

[ Remove "stored" and "are used to".  ]

> +how in-buffer completion is performed.  The following properties
> +are currently defined:
> +
> +:category
> +
> +    The symbol defining the category in
> +    `completion-category-defaults' to use for completion.  Also
> +    see `completion-category-overrides', and `completion-styles'.
> +
> +:fieldsep-re
> +
> +    The regular expression to use when scanning backwards in the
> +    buffer.  All text between point, and any preceding text
> +    matching this regular expression, will be used as the prefix
> +    for finding completion candidates.

I don't think we should limit ourselves to completion based on a prefix.
IOW, when I have:

    To: al<!>adol

where <!> represents the position of the cursor, we should consider
"aldol" as the string we're trying to complete (knowing that we're
trying to complete it from the middle).  `completion-at-point-functions`
already supports this just fine.  All we have to do is to look both
backward and forward to find the beginning and the end, rather than only
looking backward to find the beginning and always using `point` as
the end.

You currently use "\\([:,]\\|^\\)[ \t]*" as the regexp, but the ":" is
redundant because the code should already take the end of header's
regexp match as a limit on the lefthand side.  And we should strip the
spaces by hand anyway, so the regexp should just be "," (and we should
default to that if the property is not provided, as will presumably
always be the case).

> +:completions
> +
> +    The function that provides completions, and that obeys the
> +    same rules as those of `completion-at-point-functions'.

I think you forgot to update this part of the docstring: it should say
that this is a completion table rather than a function that obeys the
rules of `completion-at-point-functions`.

> +(defun message-completion-alist-set-completions (cat compl)
> +  "Set the completion function for category CAT to COMPL.
> +Modifies the value of `message-completion-alist'.  This is a
> +convenience function for use in init files."

Hmm... I'm not sure it's really convenient.  IIUC this is intended for
users to say "use EUDC for email completions instead"?  If so, maybe we
should support this more directly so users only need to set a simple
variable to get that result?

[ Then again, we already have `message-expand-name-databases` for that,
  so maybe you have some other use case in mind?  ]

> +           (let* ((completions (plist-get recipe :completions))
> +                  (beg (save-excursion
> +                         (re-search-backward (plist-get recipe :fieldsep-re))

Usually properties in plists are optional.  Admittedly, it's not always
the case, but I'd recommend signaling more explicit errors when
a property is missing.  Here if `:fieldsep-re` is missing we'll just get
an error about nil not being a string without any hint where this nil
comes from (and the naive users may end up in a wild goose chase if
they try to search the web for "how to fix: wrong-type-argument
stringp nil").
IOW I'd do something like

    (or (plist-get recipe :completions)
        (error "Missing :completions in %S" recipe))

Same for `:fieldsep-re` (not for :category since a nil category seems
like a fine default), unless you make it default to ",".

> +                         (match-end 0)))
> +                  (end (point))
> +                  (cat (plist-get recipe :category))
> +                  completion-table)
> +             (setq completion-table (if (functionp completions)
> +                                        (funcall completions beg end)
> +                                      completions))

Hmm... why do you call `completions` here?  Just use the function itself
as the completion table.  The completion machinery will call it
if/when/as needed.

> -(defun message-expand-group ()
> +(defun message-expand-group (&optional pfx-beg pfx-end)

IIUC the completion table for newgroups should be simply
`gnus-active-hashtb` instead of #'message-expand-group (which is not
a valid completion table).

`message-expand-group` should then be marked as obsolete.

> -(defun message-expand-name ()
> +(defun message-expand-name (&optional pfx-beg pfx-end)
>    (cond (message-expand-name-standard-ui
> -      (let ((beg (save-excursion
> -                      (skip-chars-backward "^\n:,") (skip-chars-forward " 
> \t")
> -                      (point)))
> -               (end (save-excursion
> -                      (skip-chars-forward "^\n,") (skip-chars-backward " \t")
> -                      (point))))
> +      (let ((beg (or pfx-beg (save-excursion
> +                                  (skip-chars-backward "^\n:,")
> +                                  (skip-chars-forward " \t")
> +                                  (point))))
> +               (end (or pfx-end (save-excursion
> +                                  (skip-chars-forward "^\n,")
> +                                  (skip-chars-backward " \t")
> +                                  (point)))))
>             (when (< beg end)
>               (list beg end (message--name-table (buffer-substring beg 
> end))))))
>       ((and (memq 'eudc message-expand-name-databases)

Again, this is not a completion table.  A completion table for names
could be:

    (completion-table-dynamic #'message--name-table)

or maybe

    (completion-table-with-cache #'message--name-table)

> @@ -8401,7 +8479,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)))))))

This is pretty hideous.  Better not do that, and let the other
(currently commented out) part of the code add the metadata from the
plist when we already have that plist info at hand.

> I have also removed message-expand-name, and message--name-table, and
> instead am calling out to EUDC. EUDC is enhanced by two new backends for
> ecomplete, and mailabbrev. Thus, in terms of functionality, end users
> should see no difference. To use the new EUDC back-ends you'll need to
> do one or both of:
>
> (require 'eudcb-ecomplete)
> (add-to-list 'eudc-server-hotlist '("localhost" . ecomplete))
>
> (require 'eudcb-mailabbrev)
> (add-to-list 'eudc-server-hotlist '("localhost" . mailabbrev))

IOW users *will* see a (bad) difference because their setup will not
work as before until they add those lines to their init file :-(

> +(defcustom message-mail-address-snarf-hook nil
> +  "Hook run to snarf email addresses.
> +This hook is run just after the message was sent as mail.
> +
> +The functions on this hook are called once for each header line
> +where email addresses were found.  They take a single argument, a
> +list of cons cells as returned by `mail-header-parse-addresses'.

The `-hook` suffix is used only for "normal hooks", i.e. hooks run by
`run-hooks`.  So this should use a name that ends in `-functions`
instead, which is the suffix normally used for those so-called "abnormal
hooks", i.e. those that take a non-empty list of arguments or whose
return value is not ignored.

This hook should probably include a function for ecomplete in its
default value, so as to preserve the existing behavior.


        Stefan