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)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 12.0

People

(Reporter: mail.twerner, Assigned: tokoe)

Details

Attachments

(1 file, 3 obsolete files)

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.
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
review?
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.
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
Assignee: nobody → tokoe
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.
Attachment #285302 - Flags: review?(richm)
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.
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?
(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.
I don't know the AB code, just the ldapsdk code - you probably want to get Mark B. or dmose to review.
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 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 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-
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
The updated patch, according to Mark's request
Attachment #312260 - Flags: superreview?
Attachment #312260 - Flags: review?
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 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+
Attachment #322913 - Flags: superreview?
Attachment #312260 - Attachment is obsolete: true
Attachment #312260 - Flags: superreview?(dmose)
Attachment #285302 - Attachment is obsolete: true
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)
Hej,

any updates on the patch? Will it make it upstream?

Ciao,
Tobias
Product: Core → MailNews Core
Anyone for superreview?
I'll bump this up my priority list; sorry for the delay.
any news on this?
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.
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 :-(
Attachment #322913 - Flags: superreview?(dmose) → superreview?(mbanner)
(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?
Attached patch Fixes bitrotSplinter Review
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+
Checked in: http://hg.mozilla.org/comm-central/rev/aaaa9a12e543
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Attachment #322913 - Attachment is obsolete: true
Attachment #322913 - Flags: superreview?(mbanner)
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.

Attachment

General

Creator:
Created:
Updated:
Size: