Closed
Bug 380636
Opened 17 years ago
Closed 12 years ago
ldap addressbook book results are not sorted when autocompleting is in action
Categories
(MailNews Core :: LDAP Integration, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 12.0
People
(Reporter: mail.twerner, Assigned: tokoe)
Details
Attachments
(1 file, 3 obsolete files)
1.48 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.3) Gecko/20070310 Iceweasel/2.0.0.3 (Debian-2.0.0.3-1) Build Identifier: 1.5.0.7 - TB is configured to use a LDAP address book - the LDAP server itself does not sort the query result - TB is not able to sort the results before displaying the to the user in the autocomplete widget Reproducible: Always Steps to Reproduce: 1. type a substring of an email address in the To: field 2. the displayed results are not sorted which is annoying when using large address books (> 10000 entries) Both the address sidebar and the standalone address book window are able to sort the LDAP query results - only the autocomplete widget is broken.
Assignee | ||
Comment 1•17 years ago
|
||
Hi, the attached patch implements the sorting of the LDAP result. I'm not sure whether my simple sorting algorithm is good enough for larger result sets, at least it is the same which is used in the addressbooksession class. Ciao, Tobias
Comment 2•16 years ago
|
||
review?
Comment 3•16 years ago
|
||
I'm no LDAP expert... can you request items in a particular sort order? Do we send the results to autocomplete all at once or individually? I don't think autocomplete allows subsequent reordering of returned items.
Updated•16 years ago
|
Assignee: mscott → nobody
Status: UNCONFIRMED → NEW
Component: Message Compose Window → MailNews: LDAP Integration
Ever confirmed: true
Product: Thunderbird → Core
QA Contact: message-compose → ldap-integration
Updated•16 years ago
|
Assignee: nobody → tokoe
Assignee | ||
Comment 4•16 years ago
|
||
Some LDAP servers support server side sorting of the result entries, however that functionality is just an extension, so it's not supported by all servers (and it's on the TODO list of OpenLDAP for several years now :(). The patch attached in comment #1 solves that problem by client side filtering. Whenever it receives new result chunks from the LDAP server, it sorts them into the internal result list of Thunderbird. We have tested it with the LDAP server of bund.de, a directory which contains address data of nearly all employees of the german government, so it's really huge. Sorting worked fine.
Assignee | ||
Updated•16 years ago
|
Attachment #285302 -
Flags: review?(richm)
Comment 5•16 years ago
|
||
Actually I was trying to work out whether it was safe to sort at this point. Your algorithm is inefficient but I haven't seen the addrbooksession one.
Assignee | ||
Comment 6•16 years ago
|
||
Hej Neil, no idea whether it is worth to tweak the insert algorithm (shouldn't be difficult thought). At least with our tests here I couldn't realize any performance problems. The network was the limiting factor... Why do you think it's not safe?
Comment 7•16 years ago
|
||
(In reply to comment #6) >Why do you think it's not safe? I don't any more ;-) but I had to study the code first to reassure myself.
Comment 8•16 years ago
|
||
I don't know the AB code, just the ldapsdk code - you probably want to get Mark B. or dmose to review.
Comment 9•16 years ago
|
||
Comment on attachment 285302 [details] [diff] [review] Patch which implements the sorting of the LDAP results I'll try and take a look at it next week.
Attachment #285302 -
Flags: review?(richm) → review?(bugzilla)
Comment 10•16 years ago
|
||
Comment on attachment 285302 [details] [diff] [review] Patch which implements the sorting of the LDAP results The general idea looks good, I've not tried it out yet, however here are some initial comments: + rv = mResultsArray->Count(&nbrOfItems); Please check the error value like we do elsewhere in this function. + nsCOMPtr<nsIAutoCompleteItem> currentItem; This would be better outside the loop. + nsresult rv = mResultsArray->QueryElementAt(insertPosition, NS_GET_IID(nsIAutoCompleteItem), + getter_AddRefs(currentItem)); There is no need to reclare rv here, please drop that. Additionally, please replace this with: currentItem = do_QueryElementAt(mResultsArray, insertPosition, &rv); This is better than the QueryElementAt function. + currentItem->GetValue(currentItemValue); This line has a tab in it, which we don't want. Please indent new/changed code at 2-space indentation as that is now the accepted standard. + if (itemValue < currentItemValue) + { + break; + } misaligned if, additionally you don't need the braces here. + rv = mResultsArray->InsertElementAt(item, insertPosition); if (NS_FAILED(rv)) { NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): " "mItems->AppendElement() failed"); The error message needs updating to say InsertElementAt rather than AppendElement.
Comment 11•16 years ago
|
||
Comment on attachment 285302 [details] [diff] [review] Patch which implements the sorting of the LDAP results This seems to work as expected, but please fix my comments in comment 10 and re-submit to me for approval.
Attachment #285302 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 12•16 years ago
|
||
Hej Mark, sorry for the late reply, I was sidetracked by another project. Thanks for your review and comments, the attached patch should include all these changes
Assignee | ||
Comment 13•16 years ago
|
||
The updated patch, according to Mark's request
Attachment #312260 -
Flags: superreview?
Attachment #312260 -
Flags: review?
Comment 14•16 years ago
|
||
Comment on attachment 312260 [details] [diff] [review] Updated patch for ldap-sorting integration Suggesting some reviewers for this patch.
Attachment #312260 -
Flags: superreview?(dmose)
Attachment #312260 -
Flags: superreview?
Attachment #312260 -
Flags: review?(bugzilla)
Attachment #312260 -
Flags: review?
Comment 15•16 years ago
|
||
Comment on attachment 312260 [details] [diff] [review] Updated patch for ldap-sorting integration Sorry for the delay in reviewing this. NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): " - "mItems->AppendElement() failed"); + "mResultsArray->InsertElementAt() failed"); The new line here should align at the same place the old line did. r=me with that fixed.
Attachment #312260 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 16•16 years ago
|
||
Attachment #322913 -
Flags: superreview?
Updated•16 years ago
|
Attachment #312260 -
Attachment is obsolete: true
Attachment #312260 -
Flags: superreview?(dmose)
Updated•16 years ago
|
Attachment #285302 -
Attachment is obsolete: true
Comment 17•16 years ago
|
||
Comment on attachment 322913 [details] [diff] [review] Updated patch which fixes issue from review+ Tobias, you need to fill in an email address when requesting review (it also helps if you mark old patches obsolete).
Attachment #322913 -
Flags: superreview? → superreview?(dmose)
Assignee | ||
Comment 18•16 years ago
|
||
Hej, any updates on the patch? Will it make it upstream? Ciao, Tobias
Updated•16 years ago
|
Product: Core → MailNews Core
Assignee | ||
Comment 19•15 years ago
|
||
Anyone for superreview?
Comment 20•15 years ago
|
||
I'll bump this up my priority list; sorry for the delay.
Comment 21•15 years ago
|
||
any news on this?
Comment 22•15 years ago
|
||
Sorry, I've been busy with Thunderbird 3 blocking stuff, I'll try and clear out my review queue after we ship. My main concern with this patch is what the performance is going to be like for the case where we have a large number of matches. It may be fine, but I'd like to look at the algorithm in detail.
Comment 23•15 years ago
|
||
Argh, so I've just remembered the work on bug 452232 (which I'm hoping we can get landed after TB 3 would obsolete the patch, and possibly make it harder to implement as well :-(
Updated•12 years ago
|
Attachment #322913 -
Flags: superreview?(dmose) → superreview?(mbanner)
Comment 24•12 years ago
|
||
(In reply to Mark Banner from comment #23) > Argh, so I've just remembered the work on bug 452232 (which I'm hoping we > can get landed after TB 3 would obsolete the patch, and possibly make it > harder to implement as well :-( Doesn't the patch in bug 452232 already sort the autocomplete results?
Comment 25•12 years ago
|
||
Neil is right, I did put a sort in that patch. So we should just get this landed - I updated the patch to apply to current trunk.
Attachment #592116 -
Flags: review+
Comment 26•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/aaaa9a12e543
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Updated•12 years ago
|
Attachment #322913 -
Attachment is obsolete: true
Attachment #322913 -
Flags: superreview?(mbanner)
Comment 27•12 years ago
|
||
Comment on attachment 592116 [details] [diff] [review] Fixes bitrot >+ currentItem = do_QueryElementAt(mResultsArray, insertPosition, &rv); This would need an #include "nsMsgUtils.h" to work with the external API.
You need to log in
before you can comment on or make changes to this bug.
Description
•