Last Comment Bug 380636 - ldap addressbook book results are not sorted when autocompleting is in action
: ldap addressbook book results are not sorted when autocompleting is in action
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: LDAP Integration (show other bugs)
: unspecified
: x86 All
: -- normal with 2 votes (vote)
: Thunderbird 12.0
Assigned To: Tobias Koenig
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-14 07:35 PDT by Torsten Werner
Modified: 2012-02-03 15:45 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch which implements the sorting of the LDAP results (1.17 KB, patch)
2007-10-17 17:50 PDT, Tobias Koenig
standard8: review-
Details | Diff | Splinter Review
Updated patch for ldap-sorting integration (1.36 KB, patch)
2008-03-28 05:38 PDT, Tobias Koenig
standard8: review+
Details | Diff | Splinter Review
Updated patch which fixes issue from review+ (1.36 KB, patch)
2008-05-29 01:57 PDT, Tobias Koenig
no flags Details | Diff | Splinter Review
Fixes bitrot (1.48 KB, patch)
2012-01-27 06:15 PST, Mark Banner (:standard8, afk until Dec)
standard8: review+
Details | Diff | Splinter Review

Description Torsten Werner 2007-05-14 07:35:14 PDT
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.
Comment 1 Tobias Koenig 2007-10-17 17:50:43 PDT
Created attachment 285302 [details] [diff] [review]
Patch which implements the sorting of the LDAP results

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 Noèl Köthe 2008-01-18 04:11:19 PST
review?
Comment 3 neil@parkwaycc.co.uk 2008-01-18 04:20:34 PST
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.
Comment 4 Tobias Koenig 2008-02-25 00:38:52 PST
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.
Comment 5 neil@parkwaycc.co.uk 2008-02-25 03:31:43 PST
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.
Comment 6 Tobias Koenig 2008-02-25 03:43:01 PST
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 neil@parkwaycc.co.uk 2008-02-25 03:44:54 PST
(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 Rich Megginson 2008-02-26 18:34:24 PST
I don't know the AB code, just the ldapsdk code - you probably want to get Mark B. or dmose to review.
Comment 9 Mark Banner (:standard8, afk until Dec) 2008-02-27 09:02:44 PST
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.
Comment 10 Mark Banner (:standard8, afk until Dec) 2008-03-18 05:54:50 PDT
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 Mark Banner (:standard8, afk until Dec) 2008-03-18 14:45:37 PDT
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.
Comment 12 Tobias Koenig 2008-03-28 05:35:58 PDT
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
Comment 13 Tobias Koenig 2008-03-28 05:38:24 PDT
Created attachment 312260 [details] [diff] [review]
Updated patch for ldap-sorting integration

The updated patch, according to Mark's request
Comment 14 neil@parkwaycc.co.uk 2008-03-28 07:26:37 PDT
Comment on attachment 312260 [details] [diff] [review]
Updated patch for ldap-sorting integration

Suggesting some reviewers for this patch.
Comment 15 Mark Banner (:standard8, afk until Dec) 2008-05-25 11:57:07 PDT
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.
Comment 16 Tobias Koenig 2008-05-29 01:57:16 PDT
Created attachment 322913 [details] [diff] [review]
Updated patch which fixes issue from review+
Comment 17 Mark Banner (:standard8, afk until Dec) 2008-05-29 11:53:09 PDT
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).
Comment 18 Tobias Koenig 2008-07-07 09:02:31 PDT
Hej,

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

Ciao,
Tobias
Comment 19 Tobias Koenig 2009-04-02 07:27:11 PDT
Anyone for superreview?
Comment 20 Dan Mosedale (:dmose) 2009-04-02 09:16:51 PDT
I'll bump this up my priority list; sorry for the delay.
Comment 21 Noèl Köthe 2009-10-29 05:56:02 PDT
any news on this?
Comment 22 Dan Mosedale (:dmose) 2009-10-29 06:12:24 PDT
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 Mark Banner (:standard8, afk until Dec) 2009-10-29 07:02:13 PDT
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 :-(
Comment 24 neil@parkwaycc.co.uk 2012-01-25 16:25:40 PST
(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 Mark Banner (:standard8, afk until Dec) 2012-01-27 06:15:50 PST
Created attachment 592116 [details] [diff] [review]
Fixes bitrot

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.
Comment 26 Mark Banner (:standard8, afk until Dec) 2012-01-27 06:27:06 PST
Checked in: http://hg.mozilla.org/comm-central/rev/aaaa9a12e543
Comment 27 neil@parkwaycc.co.uk 2012-02-03 15:45:25 PST
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.

Note You need to log in before you can comment on or make changes to this bug.