bug#59314: 29.0.50; EUDC and message-mode header completion

[Thomas Fitzsimmons <fitzsim@fitzsim.org>]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 11/16/22 14:18 PM, Thomas Fitzsimmons wrote:
>> Hi Eric,
>>
>> Thanks for filing this.
>>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> Address completion in message-mode has stopped working in master,
>>> possibly as a result of 0e25a39e69acca0324c326ea8e46b1725594bff5. This
>>> has been reported for several contact-management backends that expect to
>>> have their completions available with <TAB>.
>>>
>>> `completion-at-point-functions' contains '(eudc-capf-complete
>>> message-completion-function t) at this point -- `eudc-capf-complete'
>>> returns no matches, and no other functions in the list are consulted.
>>
>> I just checked and I didn't think the recent patch I pushed,
>> 0e25a39e6..., should have affected completion-at-point-functions.  It
>> did change the default of eudc-server-hotlist from `nil' to
>> `(("localhost" . ecomplete) ("localhost" . mailabbrev))".  I thought
>> that should only affect EUDC users who have not customized
>> eudc-server-hotlist.
>>
>> `eudc-capf-complete' was added to `message-mode' in commit
>> 620ac6735...  I'm pretty sure that commenting out this line in
>> message.el will restore prior behaviour, but I don't yet know what prior
>> behaviour should be (see below).
>>
>> (add-hook 'completion-at-point-functions #'message-completion-function nil t)
>>
>>> On gnus.general, someone using BBDB and corfu reported that this recipe
>>> fixed the problem:
>>>
>>>   (setq eudc-server-hotlist '(("localhost" . bbdb)))
>>>
>>>   (add-hook 'message-mode-hook
>>>             (lambda ()
>>>               (setq-local completion-at-point-functions
>>>                           (delq 'message-completion-function
>>>                                 completion-at-point-functions))))
>>>
>>> Someone else *not* using corfu reported that that didn't work for them.
>>> Dunno.
>>
>> I'm not sure what the out-of-the-box behaviour here is meant to be.  Can
>> you make a recipe starting from "emacs -Q" (including adding dummy email
>> addresses somewhere) that makes completion work how you want it to?  For
>> me:
>>
>> emacs -Q
>> C-x m TAB
>>
>> inserts four spaces and prints in *Messages*:
>>
>> Loading eudcb-ecomplete...done
>> Loading eudcb-mailabbrev...done
>>
>> (Those are new, due to 0e25a39e6... but I thought should be harmless.)
>
> Yuck, it's been a long time since I looked at this...
>
> In emacs -Q, message-mode `completion-at-point-functions' is:
>
> (eudc-capf-complete message-completion-function t)
>
> Actually that's what it is in my regular Emacs, as well. All I'd need
> for EBDB (and BBDB and everything else) is for
> `message-completion-function' to get called, which it isn't. I believe
> you could allow this by having `eudc-capf-complete' return nil, or have
> `eudc-capf-message-expand-name' return a `(list beg end <table>)'
> structure that includes the prop `:exclusive 'no' at the end of it. That
> would allow a fallthrough to the next function.
>
> In fact this whole message-mode thing is an impossible tangle, burritos
> within burritos, and it would be great to get rid of it altogether.
>
> `message-completion-function' already looks at where it is in the
> message buffer, and calls `message-expand-name' if it's in a "name"
> header. That function consults `message-expand-name-databases', and
> *that's* where EBDB should put its completion table, except
> `message-expand-name-databases' is hardcoded to (or 'eudc 'bbdb) for no
> reason.

Should we set `message-expand-name-databases' to (or 'eudc 'bbdb 'ebdb)?
Would that avoid the need to clobber `message-expand-name' for your use
case?  I'd be fine adding "known packages" there, as long as referring
to non-core packages doesn't break anything (which it doesn't seem to,
since BBDB is non-core, in GNU ELPA).

> So I need to clobber `message-expand-name' altogether.

When I use EUDC, I too clobber `message-mode's completion, by binding
TAB to `eudc-expand-try-all'.  Part of the effort around eudc-capf was
trying to improve the default so that this clobbering wouldn't be
necessary.  But as you point out, we're not there yet.

> And EUDC having a function on `completion-at-point-functions' is
> wrapping yet another burrito outside the existing burritos, because now
> EUDC has a completion function both inside and outside message-mode's
> own completion machinery.
>
> In fact the docstring of `eudc-capf-message-expand-name' makes it sound
> like it thinks it's being called as part of `message-expand-name', but
> now it isn't -- it's being called as part of the capf machinery. Or
> rather, it could potentially be called in both places.

> I think a half-stick of dynamite is the only real solution.

Agreed it's currently hard to navigate, but I'd prefer to take minimal
steps from what we have now, since people have configurations that
depend on the current state.

I think we should probably create a set of core "out-of-the-box"
`message-mode' completion ERT tests.  For example, given:

"emacs -Q" + EBDB + a single EBDB entry "emacs-ert-test@ebdb.gnu.org"

will "C-x m emacs TAB" work?  If it won't, will the above-suggested
`message-expand-name-databases' make it work?

Once we get "emacs-ert-test" examples for @bbdb.gnu.org,
@ecomplete.gnu.org, @mailabbrev.gnu.org, we'll be able to test how the
various completion backends interact, and I'm thinking that will help us
simplify TAB's default behaviour in `message-mode' (while preserving
backward compatibility).

Do you want to try adding a core ERT test for EBDB completion?  Optional
core tests are allowed to depend on GNU ELPA packages.

Thanks,
Thomas