User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1 there are several issues here closely related so i list them one-by-one: - ldap search filters currently used in the address book are substring search filters. these while relatively harmless against small ldap directories can be very time consuming against directories with large number of entries to search. this is mostly an issue for corporate deployments where it affects the time for the user to get desired entries and also puts unnecessary additional load on the ldap server. - displayName vs cn. displayName is not always present while cn is so it is best to look for cn attribute not displayName attribute. this makes the search more effective. - in compose new mail if ldap directory lookup is enabled it consist of cn, mail and sn attributes but is missing givenName attribute which can also improve the search. - autocomplete ignore strings with @ in them which is not right. they are valid strings like email addresses and should be allowed in the search request. there are also issues with UI not giving any visual feedback to the users that their search is in progress but i'll submit them as a separate bug. i'm not sure if two should be linked together Reproducible: Always

(In reply to comment #0) > - displayName vs cn. displayName is not always present while cn is so > it is best to look for cn attribute not displayName attribute. this > makes the search more effective. This is currently being discussed in bug 323608. IMO we need to finish discussing it there before we can accept a patch for it. > - autocomplete ignore strings with @ in them which is not right. > they are valid strings like email addresses and should be > allowed in the search request. I think this is done on purpose and is the same with non-ldap address books, cc'ing some people who might know. > there are also issues with UI not giving any visual feedback to > the users that their search is in progress but i'll submit them > as a separate bug. i'm not sure if two should be linked together IMHO Generally its best to handle issues under separate bugs/patches as it is easier to see what is happening and also makes the patches simpler to review.

Comment on attachment 211964 [details] [diff] [review] suggested fix I'm going to punt to Dan who knows much more about these issues.

Comment on attachment 215876 [details] [diff] [review] new patch against HEAD This needs dmose' email address for him to pick it up.

Comment on attachment 215876 [details] [diff] [review] new patch against HEAD Sorry for the long delay in reviewing; I'll try and do better in the future. >Index: mozilla/mail/locales/en-US/chrome/messenger/messenger.properties >=================================================================== >RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v >retrieving revision 1.17 >diff -u -8 -p -r1.17 messenger.properties >--- mozilla/mail/locales/en-US/chrome/messenger/messenger.properties 22 Feb 2006 01:21:53 -0000 1.17 >+++ mozilla/mail/locales/en-US/chrome/messenger/messenger.properties 22 Mar 2006 09:34:51 -0000 >@@ -237,17 +237,17 @@ mail.addr_book.displayName.lastnamefirst > # @V == the escaped value typed in the quick search bar in the addressbook > # > # note, changing this might require a change to SearchNameOrEmail.label > # in messenger.dtd > # > # LOCALIZATION NOTES - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true > # "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))" > # >-mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)) >+mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,bw,@V)(DisplayName,bw,@V)(FirstName,bw,@V)(LastName,bw,@V)) This seems like a reasonable change to me, but I'd be interested in Standard8's and bienvenu's thoughts on this, as it's going to effect all addressbooks, not just LDAP ones. If we do decide that this is the right thing to do, I'd suggest tweaking the example in the comment as well. >-pref("ldap_2.servers.default.attrmap.DisplayName", "cn,commonname"); >+pref("ldap_2.servers.default.attrmap.DisplayName", "cn,displayName,commonname"); We pretty much decided over in <https://bugzilla.mozilla.org/show_bug.cgi?id=323608#c10> that we didn't want to do this. If you wanted to file a separate bug on giving the addressbook a concept of different field/attribute mappings for search & display, that would be a fine thing, since in the current world, if we don't search on displayName, we'll never actually use it for displaying. >Index: mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp,v >retrieving revision 1.45 >diff -u -8 -p -r1.45 nsLDAPAutoCompleteSession.cpp >--- mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp 3 Feb 2006 14:18:09 -0000 1.45 >+++ mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp 22 Mar 2006 09:34:54 -0000 >@@ -68,17 +68,17 @@ static PRLogModuleInfo *sLDAPAutoComplet > // version. > // > NS_IMPL_THREADSAFE_ISUPPORTS3(nsLDAPAutoCompleteSession, > nsIAutoCompleteSession, nsILDAPMessageListener, > nsILDAPAutoCompleteSession) > > nsLDAPAutoCompleteSession::nsLDAPAutoCompleteSession() : > mState(UNBOUND), >- mFilterTemplate("(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))"), >+ mFilterTemplate("(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*)(givenName=%v1*%v2-*))"), Excellent idea. >- // ignore the empty string, strings with @ in them, and strings >- // that are too short >+ // ignore the empty string and strings that are too short > // > if (searchString[0] == 0 || >- nsDependentString(searchString).FindChar(PRUnichar('@'), 0) != >- kNotFound || > nsDependentString(searchString).FindChar(PRUnichar(','), 0) != > kNotFound || The original idea behind was that if you were typing a full-email address, that meant that you wanted to type it yourself. In organizations where the LDAP server only serves one domain that makes sense. In other cases, not so much. In general, this seems like a reasonable change to me, but I'd be interested in hearing thoughts from Standard8 and bienvenu on this too.

(In reply to comment #6) > >-pref("ldap_2.servers.default.attrmap.DisplayName", "cn,commonname"); > >+pref("ldap_2.servers.default.attrmap.DisplayName", "cn,displayName,commonname"); > > We pretty much decided over in > <https://bugzilla.mozilla.org/show_bug.cgi?id=323608#c10> that we didn't want > to do this. > If you wanted to file a separate bug on giving the addressbook a concept of > different field/attribute mappings for search & display, that would be a fine > thing, since in the current world, if we don't search on displayName, we'll > never actually use it for displaying. when i submitted the patch originally it was still undecided at the time and i kept displayName as a safety catch just in case. i'm fine with removing it now as it is no longer required. once you guys decide on the rest of the changes i shall submit a new patch so let me know.

> The original idea behind was that if you were typing a full-email address, > that meant that you wanted to type it yourself. In organizations where the > LDAP server only serves one domain that makes sense. In other cases, not so > much. In general, this seems like a reasonable change to me, but I'd be > interested in hearing thoughts from Standard8 and bienvenu on this too. In my (and my wife's) experience, which involves emailing folks from a variety of organizations, autocompleting on a string containing a @ would be useful (as long as it also applied to local addressbooks).

nsAbAutoCompleteSession::OnStartLookup would need a similar change to allow autocomplete with '@' in the string. And the autocomplete widget itself might need to be changed - I'll go look at that.

I'd definitely prefer autocomplete to keep working after I typed the '@'. I'm going to submit a patch to do that for local ab's in bug 200804 - the autocomplete part of this patch could end up in that other bug, if people want...

autocomplete after @ is now fixed for local ab's, so fixing it for ldap would have the added benefit of consistency :-)

time for another pass?

Comment on attachment 215876 [details] [diff] [review] new patch against HEAD Patch has bitrotted. $ patch --dry-run < ~/Desktop/tbTestPatches/patch-327263.diff patching file messenger.properties Hunk #1 FAILED at 237. 1 out of 1 hunk FAILED -- saving rejects to file messenger.properties.rej can't find file to patch at input line 34 Perhaps you should have used the -p or --strip option? The text leading up to this was: -------------------------- |Index: mozilla/mailnews/mailnews.js |=================================================================== |RCS file: /cvsroot/mozilla/mailnews/mailnews.js,v |retrieving revision 3.262 |diff -u -8 -p -r3.262 mailnews.js |--- mozilla/mailnews/mailnews.js 2 Mar 2006 15:51:00 -0000 3.262 |+++ mozilla/mailnews/mailnews.js 22 Mar 2006 09:34:52 -0000 -------------------------- File to patch:

Anton, any chance for a new patch?

Anton indicates no "spare cycles right now"

I'll try to salvage this.