bug#19678: [PATCH] EUDC does not support BBDB 3.x

[Thomas Fitzsimmons]

Sergio Durigan Junior <sergiodj@sergiodj.net> writes:

> On Monday, January 26 2015, Thomas Fitzsimmons wrote:
>
>>> Almost there...  The patch doesn't work as-is because
>>> 'eudc-bbdb-current-return-attributes' is set to '(firstname lastname
>>> mail) for me when eudc-bbdb-format-record-as-result is called.  It means
>>> that, in the while loop, it will try to call eq/memq against 'mail, and
>>> it fails.
>>
>> Hmm, I thought those get converted; in any case, I wasn't seeing that
>> problem.  Do you have any customizations of the relevant variables?  Do
>> you have my latest EUDC/LDAP changes from master tip?
>
> I am using git HEAD to test and develop, so I think I do have your
> changes.
>
> As for my customizations, I'm almost sure they're the reason for me
> seeing the errors.  What I have here is:
>
>   (eudc-protocol-set 'eudc-inline-expansion-format '("%s %s <%s>" firstname 
> name mail)
>                      'bbdb)

OK, yes, that explains why you're seeing an error and I'm not.

Though I can understand why you're doing it this way (see below), I
think calling eudc-protocol-set directly in this case is a
misconfiguration; it bypasses the "defcustom" way of doing things.
Prior to my patches, the way this was supposed to work is that the user
customized eudc-inline-expansion-format using generic EUDC symbols for
the fields.  In your case, this would have been:

(customize-set-variable 'eudc-inline-expansion-format
                        '("%s %s <%s>" firstname name email))

i.e., email not mail, and with no protocol specified.  Then EUDC would
translate those to the fields used by the specific backend.  Then if you
were using an LDAP server and a BBDB "server", you'd get the same
results from both without having to figure out the "email" field for
each of them.

All that said, my patches fixed the default to be exactly what you're
configuring.  Now you can just remove that line and it'll do exactly
what you want.

>   (eudc-protocol-set 'eudc-inline-query-format '((mail)
>                                                  (firstname)
>                                                  (lastname)
>                                                  (firstname lastname)
>                                                  (aka))
>                      'bbdb)

I also changed this default to be:

'((email)
  (firstname)
  (firstname name))

The aka is where eudc-protocol-set seems to make most sense; after all,
aka is BBDB-specific.  But EUDC backends will just return no results for
fields they don't know about, so it's actually safe to just put aka in
eudc-inline-query-format via customization, without resorting to
eudc-protocol-set.  The LDAP backend will ignore it, the BBDB backend
will interpret and attempt it:

(customize-set-variable 'eudc-inline-query-format '((email)
                                                    (firstname)
                                                    (name)
                                                    (firstname name)
                                                    (aka))

The manual should maybe explicitly mention this, but then we'd have to
expose all fields from all backends directly to the user for
configuration, which may confuse things even more.

In general I don't like this aspect of EUDC: how confusing it is to
configure.  It's made even worse by the fact that even when configured
properly it can still fail because of bugs.  I've tried to simplify the
configuration by providing better defaults, and I'm also trying to fix
the bugs.

>> Can we step back a bit and make sure we're doing the same tests?  BBDB
>> 2.x is tricky because it is provided by the distro, in my case Fedora.
>> Let's focus on testing BBDB 3.x so that I can replicate the exact same
>> issue that you're seeing.
>>
>> Can you revert our patch, then:
>>
>> 1) Checkout and build Emacs revision
>>    03a20dc9519616359bfa1b77fd4b31e1963c8bd4 from
>>    git://git.savannah.gnu.org/emacs.git
>>
>>    This revision has a bunch of my EUDC/LDAP updates in it.
>
> I'm buying an even newer revision from git, FWIW.
>
>> 2) Download
>>    http://download.savannah.gnu.org/releases/bbdb/bbdb-3.1.2.tar.gz
>>
>> 3) Untar and build the ELPA package:
>>    export EMACS=<your-emacs-build-dir>/src/emacs
>>    ./configure && make elpa
>>    unset EMACS
>>
>> 4) In the emacs src directory:
>>    mkdir test-home
>>
>> 5) HOME=`pwd`/test-home ./emacs -Q
>>
>> 6) M-x package-install-file
>>
>>    bbdb-3.1.2.tar (the one built in step 3)
>>
>> 7) M-x bbdb-create
>>
>>    Name: Test User
>>    Network Address: test@gnu.org
>>
>> 8) C-x s
>>
>>    to save .bbdb
>>
>> 9) M-: (eval-after-load "message"
>>         '(define-key message-mode-map (kbd "TAB") 'eudc-expand-inline))
>>
>> 10) M-: (setq debug-on-error 't)
>>
>> 11) C-x m
>>
>> 12) Tes[TAB]
>>
>>     (no server, bbdb protocol)
>>
>> Without my patch, I get:
>>
>> Debugger entered--Lisp error: (void-function bbdb-record-net)
>>   (bbdb-record-net record)
>>   eval((bbdb-record-net record))
>>
>>   eudc-bbdb-format-record-as-result(["Test" "User" nil nil nil nil nil
>>   ("test@gnu.org") ((creation-date . "2015-01-26 21:56:26 +0000")
>>   (timestamp . "2015-01-26 21:56:26 +0000")) ["Test User" "User, Test"
>>   nil ("test@gnu.org") nil #<marker at 64 in bbdb>]])
>>   
>>   mapcar(eudc-bbdb-format-record-as-result (["Test" "User" nil nil nil
>>   nil nil ("test@gnu.org") ((creation-date . "2015-01-26 21:56:26
>>   +0000") (timestamp . "2015-01-26 21:56:26 +0000")) ["Test User" "User,
>>   Test" nil ("test@gnu.org") nil #<marker at 64 in bbdb>]]))
>>   
>>   eudc-bbdb-query-internal(((firstname . "Tes")) (firstname lastname
>>   net))
>>   
>>   eudc-query(((firstname . "Tes")) (firstname lastname net))
>>   eudc-expand-inline()
>>   funcall-interactively(eudc-expand-inline)
>>   call-interactively(eudc-expand-inline nil nil)
>>   command-execute(eudc-expand-inline)
>>
>> With my patch (0001-EUDC-Support-BBDB-3.patch), it works.  eudc-query
>> gets called with 'net, not 'mail.
>>
>> If that works for you, can you try to replicate the other error you're
>> seeing when my patch is applied, in this same minimal environment, and
>> paste the testing steps and the backtrace you get?
>
> If you insist, I can do this test later (unfortunately I will be very
> busy these next days). 

No need.

> Meanwhile, if you could check that my configuration is what triggers
> the failure, I'd appreciate.

Done.

> Also, IMHO, the final patch makes sense to me, even if there are still
> other issues to be fix on EUDC.

I'd like to keep the attribute symbols "private" in the backends, and
encourage people to use the generic EUDC symbol names instead (plus
hidden extras like aka, if they want) using the standard customization
procedures.  In which case the 'net stuff can stay as is in the BBDB
backend.  Can you try the configuration changes I've outlined above,
along with 0001-EUDC-Support-BBDB-3.patch, and confirm that they work
for you?

I'll credit you in the ChangeLog too since we both worked on the patch.
Let me know when your copyright assignment goes through, and I'll push
it after that.

Thanks,
Thomas