Autocomplete does not work in address book for lists

RESOLVED FIXED in Thunderbird 31.0

Status

MailNews Core
Address Book
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gerv, Assigned: tessarakt)

Tracking

({regression})

Thunderbird 31.0
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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

Updated

4 years ago
Keywords: regression, regressionwindow-wanted

Comment 1

4 years ago
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
status-thunderbird27: --- → unaffected
status-thunderbird28: --- → affected
status-thunderbird29: --- → affected
tracking-thunderbird28: --- → ?
tracking-thunderbird29: --- → ?
Component: Address Book → Address Book
Keywords: regressionwindow-wanted
Product: Thunderbird → MailNews Core
(Assignee)

Comment 2

4 years ago
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).
(Reporter)

Comment 3

4 years ago
Alice0775: thanks for finding the regression range!

Gerv
(Assignee)

Updated

4 years ago
Assignee: nobody → blog
(Assignee)

Comment 4

4 years ago
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" ...
(Assignee)

Comment 5

4 years ago
Created attachment 8350990 [details] [diff] [review]
Make LDAP and address book autocomplete work again for places other than compose (v1)
(Assignee)

Updated

4 years ago
Attachment #8350990 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8350990 - Flags: review?(neil)
(Assignee)

Updated

4 years ago
Attachment #8350990 - Flags: review?(mschroeder)
(Assignee)

Comment 6

4 years ago
(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

4 years ago
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+
(Reporter)

Comment 9

4 years ago
mconley: ping on your regression review here?

Gerv
(Assignee)

Comment 10

4 years ago
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+?
(Reporter)

Comment 11

4 years ago
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+
(Assignee)

Comment 14

4 years ago
And I repeat the question from Comment 10 ...
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 16

4 years ago
(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.?
(Reporter)

Comment 17

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

Gerv

Comment 18

4 years ago
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
Last Resolved: 4 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.

Comment 23

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

Comment 24

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

Comment 25

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

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

Comment 26

4 years ago
Try looked good. Landed as https://hg.mozilla.org/comm-central/rev/9d9a9045b462 -> FIXED
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Thunderbird 30.0 → Thunderbird 31.0

Comment 27

4 years ago
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?
status-seamonkey2.26: --- → affected
status-seamonkey2.27: --- → affected
status-seamonkey2.28: --- → fixed

Comment 28

4 years ago
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+

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora/comm-beta]
https://hg.mozilla.org/releases/comm-aurora/rev/503d3e033441
https://hg.mozilla.org/releases/comm-beta/rev/c3ba9d95d05a
status-seamonkey2.26: affected → fixed
status-seamonkey2.27: affected → fixed
status-thunderbird28: affected → ---
status-thunderbird29: affected → fixed
status-thunderbird30: --- → fixed
tracking-thunderbird28: ? → ---
tracking-thunderbird29: ? → ---

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora/comm-beta]

Updated

3 years ago
Blocks: 1125506
You need to log in before you can comment on or make changes to this bug.