952735 - Autocomplete does not work in address book for lists

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Gervase Markham [:gerv]]

Earlybird 28.0a2, 2012-12-20, Ubuntu Linux 13.10.

STR:
1) Create a new list
2) Start typing an email address known to Thunderbird

Expect: autocomplete

Actual: the following errors in the Error Console:

Timestamp: 21/12/13 09:15:58
Error: JSON.parse: unexpected end of data
Source File: resource://gre/components/nsAbAutoCompleteSearch.js
Line: 349

Timestamp: 21/12/13 09:15:58
Error: JSON.parse: unexpected end of data
Source File: resource://gre/components/nsAbLDAPAutoCompleteSearch.js
Line: 164

Gerv

Comment 1

[Alice0775 White]

Regression window(common-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/7b014f0f3b03
http://hg.mozilla.org/comm-central/rev/34f8c2ebcc73
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Thunderbird/28.0a1 ID:20131113030201
Bad:
http://hg.mozilla.org/mozilla-central/rev/7b014f0f3b03
http://hg.mozilla.org/comm-central/rev/2523fe46615f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Thunderbird/28.0a1 ID:20131114030203
Pushlog:
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=34f8c2ebcc73&tochange=2523fe46615f

Regressed by:
2523fe46615f	Jens Mueller — Bug 61491 - Autocomplete newsgroup names (based on a patch by Jim Porter). r=neil/mconley, moa=neil, sr=Standard8; small followup fix for test: r=jcranmer CLOSED TREE

Comment 2

[Jens Müller (:tessarakt)]

Thanks for noticing this. The autocompletesearchparam attribute has to be added here (like here: https://hg.mozilla.org/comm-central/diff/2523fe46615f/mail/components/compose/content/messengercompose.xul), and in the corresponding code, "idKey", "accountKey" and "type" have to be set accordingly, like here: https://hg.mozilla.org/comm-central/diff/2523fe46615f/mail/components/compose/content/MsgComposeCommands.js and here: https://hg.mozilla.org/comm-central/diff/2523fe46615f/mail/components/compose/content/addressingWidgetOverlay.js

There are probably some more related bugs like this one. http://mxr.mozilla.org/comm-central/search?string=autocompletesearch%3D%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central :

http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml#40
http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abEditListDialog.xul#60
http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abMailListDialog.xul#70
http://mxr.mozilla.org/comm-central/source/suite/mailnews/addrbook/abListOverlay.xul#68

(everything that uses the autocompletesearch values addrbook or ldap (or mydomain, but that is not used anywhere except in the compose window, which has already been adapted in bug 61491).

Comment 3

[Gervase Markham [:gerv]]

Alice0775: thanks for finding the regression range!

Gerv

Comment 4

[Jens Müller (:tessarakt)]

It appears that the "API" of autocomplete modules (i.e., the properties of the object that is JSON-serialized in autocompletesearchparam) needs to be documented somewhere. What would be a proper place for such documentation?

Concerning the fix: Another idea is to just add an empty (i.e., "{}") autocompletesearchparam attribute and make the autocomplete modules for ldap and addrbook NOT ignore the search when the "type" attribute is not set. That would be a minimal change, and we need not think about proper (possibly new) values for "type" ...

Comment 5

[Jens Müller (:tessarakt)]

Attachment: Make LDAP and address book autocomplete work again for places other than compose (v1)

Comment 6

[Jens Müller (:tessarakt)]

(Requesting review from mconley for the general mailnews and specific Thunderbird changes, from Neil for Suite changes, and from mschroeder for Calendar changes)

Comment 7

[[e-mail]]

Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

Supporting a blank type is equivalent to supporting a new type except worse because people will then pass a blank type without realising what it means. My preference would be to add a new addr_list type which makes it clear what's going on.

>+            (!params.idKey || dir.useForAutocomplete(params.idKey))) {
useForAutocomplete already copes with an empty key. (Actually the LDAP directory is the only one that cares about the key.)

>-                 autocompletesearch="addrbook ldap" timeout="300" maxrows="4"
>+                 autocompletesearch="addrbook ldap"
>+                 autocompletesearchparam="{}" timeout="300" maxrows="4"
Just put the new attribute on its own line please.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

r+ for calendar.

Comment 9

[Gervase Markham [:gerv]]

mconley: ping on your regression review here?

Gerv

Comment 10

[Jens Müller (:tessarakt)]

I thought I still have to add the new address type anyway (did not have time yet :-/ ). Or wasn't that a condition of neil's r+?

Comment 11

[Gervase Markham [:gerv]]

This is a little frustrating - I reported this significant functional regression over a month ago, we've had a patch for almost that long, and I go to create a new mailing list today and _still_ no autocomplete.

mconley: if you don't have bandwidth to review, can you nominate someone else?

Thanks,

Gerv

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I apologize - my TB backlog is getting quite long. Let me give this a quick peek in between Australis patches...

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

Review of attachment 8350990 [details] [diff] [review]:
-----------------------------------------------------------------

The mail/ changes look fine to me. Let's land this puppy.

Thanks, and sorry for the delay!

Comment 14

[Jens Müller (:tessarakt)]

And I repeat the question from Comment 10 ...

Comment 15

[[e-mail]]

I would have thought you might have been able to implement a new type by now. If you won't have the time to do it promptly, it would probably be better to land your current patch and address the type issue in a follow-up provided that it can also get backported to TB28.

Comment 16

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #15)
> I would have thought you might have been able to implement a new type by
> now. If you won't have the time to do it promptly, 

Actually, this is the case.

> it would probably be
> better to land your current patch and address the type issue in a follow-up
> provided that it can also get backported to TB28.

OK.

So what is required from me now?

Separate patches for comm-aurora and comm-central?

Are the reviews sufficient, or is there a need for any sr etc.?

Comment 17

[Gervase Markham [:gerv]]

I think 3 reviews is surely enough. Can we land this, please? :-)

Gerv

Comment 18

[Magnus Melin [:mkmelin]]

I went a head and landed this. 
https://hg.mozilla.org/comm-central/rev/28d17c955e99 -> FIXED

As for comm-aurora, patches can usually be applied there as is, but i don't know if it's deemed necessary to land there.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This may have broken a test, please fix or backout.

TEST-INFO | /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js | running test ...
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
TEST-PASS | /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js | [run_test : 122] [xpconnect wrapped nsIAutoCompleteSearch] == [xpconnect wrapped nsIAutoCompleteSearch]
TEST-PASS | /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js | [run_test : 123] "abc" == "abc"
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js | 4 == 3 - See following stack:
JS frame :: /builds/slave/test/build/xpcshell/tests/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js :: run_test :: line 124
JS frame :: /builds/slave/test/build/xpcshell/head.js :: _execute_test :: line 375
JS frame :: -e :: <TOP_LEVEL> :: line 1
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
TEST-INFO | (xpcshell/head.js) | exiting test

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Per jcranmer's suggestion I shouldn't have reopened without backing out the changeset. I've done this now:

https://hg.mozilla.org/comm-central/rev/a2733a761f2b

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Jens, do you think you could look into this bug again? I've had a calendar bug report about this one too.

Comment 23

[Peter Lairo]

The Summary is a bit inaccurate? I get no autocomplete on *individual* addresses, not just Lists.

Comment 24

[Christoph Kaulich]

Yes, the calendar bug 979196 is also about adding single partipants (email addresses) to an event.

Comment 25

[Magnus Melin [:mkmelin]]

Sent a (probable) fix to try. https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=927664bd5f9c

The (!params.idKey || ) change was wrong.

Comment 26

[Magnus Melin [:mkmelin]]

Try looked good. Landed as https://hg.mozilla.org/comm-central/rev/9d9a9045b462 -> FIXED

Comment 27

[rsx11m]

Per http://forums.mozillazine.org/viewtopic.php?f=5&t=2816333 this also affects SeaMonkey's current releases, thus probably 2.26b1 and 2.27a2 as well.

Can you nominate this for aurora and beta channels, assuming it works well on trunk?

Comment 28

[rsx11m]

Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

Well, so this appears to run on trunk and the patch applies to the branches, hence I'll give it a try as nobody else does.

[Approval Request Comment]
Regression caused by (bug #): bug 61491
User impact if declined: autocomplete doesn't work correctly in certain cases
Testing completed (on c-c, etc.): patch applies against comm-{aurora,beta}
Risk to taking this patch (and alternatives if risky): apparently low

We still seem to be within Thunderbird's 29.0 beta cycle, nothing tagged yet.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

a=Standard8

Comment 30

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/503d3e033441
https://hg.mozilla.org/releases/comm-beta/rev/c3ba9d95d05a