Closed Bug 952735 Opened 6 years ago Closed 6 years ago

Autocomplete does not work in address book for lists

Categories

(MailNews Core :: Address Book, defect, major)

defect
Not set
major

Tracking

(thunderbird27 unaffected, thunderbird29 fixed, thunderbird30 fixed, seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed)

RESOLVED FIXED
Thunderbird 31.0
Tracking Status
thunderbird27 --- unaffected
thunderbird29 --- fixed
thunderbird30 --- fixed
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed
seamonkey2.28 --- fixed

People

(Reporter: gerv, Assigned: tessarakt)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
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
Blocks: 61491
Product: Thunderbird → MailNews Core
Alice0775: thanks for finding the regression range!

Gerv
Assignee: nobody → blog
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" ...
Attachment #8350990 - Flags: review?(mconley)
Attachment #8350990 - Flags: review?(neil)
Attachment #8350990 - Flags: review?(mschroeder)
(Requesting review from mconley for the general mailnews and specific Thunderbird changes, from Neil for Suite changes, and from mschroeder for Calendar changes)
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.
Attachment #8350990 - Flags: review?(neil) → review+
Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

r+ for calendar.
Attachment #8350990 - Flags: review?(mschroeder) → review+
mconley: ping on your regression review here?

Gerv
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+?
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
I apologize - my TB backlog is getting quite long. Let me give this a quick peek in between Australis patches...
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!
Attachment #8350990 - Flags: review?(mconley) → review+
And I repeat the question from Comment 10 ...
Flags: needinfo?(neil)
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.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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.?
I think 3 reviews is surely enough. Can we land this, please? :-)

Gerv
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Duplicate of this bug: 979196
Jens, do you think you could look into this bug again? I've had a calendar bug report about this one too.
The Summary is a bit inaccurate? I get no autocomplete on *individual* addresses, not just Lists.
Yes, the calendar bug 979196 is also about adding single partipants (email addresses) to an event.
Sent a (probable) fix to try. https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=927664bd5f9c

The (!params.idKey || ) change was wrong.
Try looked good. Landed as https://hg.mozilla.org/comm-central/rev/9d9a9045b462 -> FIXED
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Thunderbird 30.0 → Thunderbird 31.0
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 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.
Attachment #8350990 - Flags: approval-comm-beta?
Attachment #8350990 - Flags: approval-comm-aurora?
Comment on attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)

a=Standard8
Attachment #8350990 - Flags: approval-comm-beta?
Attachment #8350990 - Flags: approval-comm-beta+
Attachment #8350990 - Flags: approval-comm-aurora?
Attachment #8350990 - Flags: approval-comm-aurora+
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora/comm-beta]
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora/comm-beta]
Blocks: 1125506
You need to log in before you can comment on or make changes to this bug.