allow users to specify which LDAP directories and local ABs the want to use for autocomplete and/or show in the AB window.

Some rough ideas can be see here: http://www.mozilla.org/mailnews/specs/addressbook/#Prefs

Marking nsbeta1 so that users have more control over their address book.

*** Bug 147734 has been marked as a duplicate of this bug. ***

*** Bug 191076 has been marked as a duplicate of this bug. ***

Please look at bug# 191809 where he asks for the ability to specify which LDAP dir and local AB to use per account. He thinks that the global setting make it more confusing and is not necessary.

adt: nsbeta1-

This is using a tree with a treecol type="checkbox"

This is a work in progress patch. What works: * Selecting which addressbooks (local and ldap) can be used for autocompletion both globally * Autocompletion globally on multiple ldap addressbooks and local addressbooks What still needs to be done: * Autocompletion per account * Per account preference setting * Migration of prefs * Help updates

This patch has a few updates in to do with passing in the autocomplete list through to the autocomplete session - no testing done as yet.

Changes since v0.1: * Now deals with per-identity local address books. * Correctly removes unused sessions from gLDAPSessions and autoCompleteWidget. * Updates non-LDAP autocomplete sessions when changes are made to which local addressbooks are being used. Work still to be done: * Extensions/smime changes * Fix per-account/identity prefs to be completely disabled when set to global * More tidy up of MsgComposeCommands.js code * Migration of prefs * Help updates

Comment on attachment 219554 [details] [diff] [review] Work in Progress patch v0.1b Ok, I've taken a very brief look, so apologies if I've misunderstood some of it. Here's a few comments on some of the obvious things I've spotted. +function LoadDirectories(aListbox) I think you should actually be able to drop this function completely - as long as make sure that initLDAPPrefsService is called early/on load. You shouldn't need to fill out aListBox with it as that will be done via RDF - Do you realise the function only does LDAP items anyway? (I think). + for (index = 0; index < gAvailDirectories.length; index++) I'm not sure I like the usage of gAvailDirectories, I'd rather the value/id was obtained from the rdf value in the list box and not from a pre-stored list. +function addressBookIdToName(aId, aIsRemote) This function looks dangerous. Is there going to be a case where we could have a remote address book with no filename? I'm not sure what outlook abs do for that. + filename = gPrefInt.getComplexValue(gAvailDirectories[index].value + ".filename", + Components.interfaces.nsISupportsString).data; Interesting. I didn't think we normally treated the filename as a complex value. function ReleaseGlobalVariables() { - gLDAPSession = null; + gLDAPSessions = new Object(); I'd have expected that to remain as null. -pref("mail.identity.default.directoryServer",""); +pref("mail.identity.default.addressBooks",""); I think you need to make this autoCompleteAddressBooks or something like that.

(In reply to comment #11) > (From update of attachment 219554 [details] [diff] [review] [edit]) > Ok, I've taken a very brief look, so apologies if I've misunderstood some of > it. Here's a few comments on some of the obvious things I've spotted. > > +function LoadDirectories(aListbox) > > I think you should actually be able to drop this function completely - as long > as make sure that initLDAPPrefsService is called early/on load. You shouldn't > need to fill out aListBox with it as that will be done via RDF - Do you realise > the function only does LDAP items anyway? (I think). Despite getServerList being a function in gLDAPPrefsService it does return both LDAP and non-LDAP items, just searches the pref tree for branches at a certain level. > > + for (index = 0; index < gAvailDirectories.length; index++) > > I'm not sure I like the usage of gAvailDirectories, I'd rather the value/id was > obtained from the rdf value in the list box and not from a pre-stored list. > At the moment, without adding another RDF resource uri, I cannot see a way of doing it. DirUri returns something like moz-abmdbdirectory://abook-5.mab for local ABs whereas for LDAP ABs it is something like moz-abmdbdirectory://ldap_2.servers.foo so for local ABs need to search for the correct pref branch name but it can be worked out for LDAP ABs. > +function addressBookIdToName(aId, aIsRemote) > > This function looks dangerous. Is there going to be a case where we could have > a remote address book with no filename? I'm not sure what outlook abs do for > that. Well with no filename the function returns null, so I could always do a null check. > > + filename = gPrefInt.getComplexValue(gAvailDirectories[index].value + > ".filename", > + > Components.interfaces.nsISupportsString).data; > > Interesting. I didn't think we normally treated the filename as a complex > value. Not sure where that came from , should getCharPref or something like that. > > function ReleaseGlobalVariables() > { > - gLDAPSession = null; > + gLDAPSessions = new Object(); > > I'd have expected that to remain as null. > > -pref("mail.identity.default.directoryServer",""); > +pref("mail.identity.default.addressBooks",""); > > I think you need to make this autoCompleteAddressBooks or something like that. > Agreed.

Changes since v0.1b: * Made some of suggested changes * Started work on smime extension changes * Started work on pref migration changes

Changes since v0.1d: * Added equivalent code for Thunderbird * Added and used new data source DirPrefId * Switched to using RDF for directory list * Added some more migration code * Changed from ldap_2.autoComplete.addressBooks to mail.autoComplete.addressBooks for pref Still to be done: * Migration code for per-identity prefs * Fix checkboxes in lists not being disabled when listbox is disabled * Help changes

*** Bug 338563 has been marked as a duplicate of this bug. ***

In addition to supporting features like autocomplet, LDAP servers are also used to retrieve the encryption certificate of intended recipients. We have deployments were we would like to be able to select one or more LDAP servers to be used for autocomplete addressing, and at the same time, a potentially different set of LDAP servers to be used for certificate retrieval. Scenario 1: I work at Company A, and I want to set my client to use the corporate LDAP server for autocomplete addressing and for certificate retrieval. At the same time, I want to use not only A's LDAP server for certificate retrieval, but also Company B's public-facing LDAP server. I don't want to use Company B's server for cert retrieval. They are a much, much larger company, and I only send mail to a small number of their employees. I don't want their employees to pollute my autocomplete listings. Scenario 2: I work at a huge multinational company with many divisions. There are many directory servers across the various divisions and countries with different schemas and different employees, despite the best efforts to centralize. I need to be able to configure the mail client to use a few for autocomplete, and a different set for certificate retrieval. How can the UI make this selection process simple to understand? Add another column to the proposed UI here? http://www.mozilla.org/mailnews/specs/addressbook/images/LdapSel2.gif (which is from http://www.mozilla.org/mailnews/specs/addressbook/#Prefs)

Changes since v0.1h: * More pref migration code added - it even compiles! * Help changes addded * Listbox disabling spun off into bug 338156 Still to be done: * Per-identity pref migration * Indepth testing

Sorry previous version of patch v0.1i had one outdated diff in.

(In reply to comment #18) > Created an attachment (id=223120) [edit] > Work in Progress patch v0.1i (correct version) > > Sorry previous version of patch v0.1i had one outdated diff in. > I believe you asked me for some comments... Index: mailnews/addrbook/prefs/resources/content/pref-directory.js + var aLDAPPrefsService = null; It's not an attribute, so I think you can drop the 'a' +function initPrefService() Do you really need this function? Could you just do var gPrefInt = Components.... at the top? At the very least, I think you should combine initPrefService and initRDFService into one function. Whilst we're here can you make it gPrefBranch? gPrefInt implies an old interface. + acValue = acList.getAttribute("value"); acList.value should work here shouldn't it? This applies in a few places. +function updatedAutocompleteList(aList, aBook, aShouldAdd) That doesn't sound right for a function name, should it be just updateAutocompleteList? (and maybe a capital c?) Ok, I'm about half way through the js file, I'll add more comments soon.

(In reply to comment #19) > (In reply to comment #18) > > Created an attachment (id=223120) [edit] > > Work in Progress patch v0.1i (correct version) > > > > Sorry previous version of patch v0.1i had one outdated diff in. > > More comments: + <button id="editButton" label="&editDirectories.label;" + prefstring="pref.ldap.disable_button.edit_directories" + oncommand="onEditDirectories();" + accesskey="&editDirectories.accesskey;"/> Do we still need this option and the associated dialog? Considering what this bug is doing we'd possibly end up having buttons for ldap, outlook, normal and others... May be worth a follow-up bug to consider it (or one to be implemented before). + var ldapList; + try + { + ldapList = gPrefInt.getComplexValue("mail.autoComplete.addressBooks", I think ldapList would be better named autoCompleteList or something like that. +nsresult nsAbAutoCompleteSession::NeedToSearchDirectories(nsIPrefBranch *aPrefs, PRBool *aNeedToSearch) +nsresult nsAbAutoCompleteSession::SearchDirectories(nsIPrefBranch *aPrefs, nsAbAutoCompleteSearchString* searchStr, PRBool searchSubDirectory, nsIAutoCompleteResults* results) I know most of this is what we do currently, but I really don't like the way this file is structured, especially with working out online/offline status. I think my main concern with it is that it is relying too much on using the back-end prefs directly to access directories, when really it should be using the xpcom interfaces, preferably all through nsIAbDirectory. That way we would be able to add a new directory type with autocomplete enabled without having to change this file. I'm not sure if that's outside the scope of this patch or not. I think if whatever we do, I would still like to see the pref storage as mozabdirectory://.... or whatever, even if for now, the code that uses those picks up the dirpref string from the nsIAbDirectory. I think it would provide us with much greater flexibility for the future, without having to change those prefs again later.

Changes since v0.1i: * Started move to using addressbook uri instead of addressbook prefname for what is stored in the autocompleteaddressbook list. * Started move to using values stored in RDF instead of prefs where possible.

Comment on attachment 228526 [details] [diff] [review] Work in Progress patch v0.1k I'm having some trouble getting this to compile - I think you've missed out changes in mailnews/addrbook/public in the patch. Also nsAbAutoCompleteSession.h needs a "class nsIRDFService". Finally I get a compile error: -nsresult nsAbAutoCompleteSession::SearchDirectory(const nsACString& aURI, nsAbAutoCompleteSearchString* searchStr, PRBool searchSubDirectory, nsIAutoCompleteResults* results) +nsresult nsAbAutoCompleteSession::SearchDirectory(nsIAbDirectory *aDirectory, ... - directory = do_QueryInterface(item, &rv); + aDirectory = do_QueryInterface(item, &rv); the error being: nsAbAutoCompleteSession.cpp:592: error: cannot convert ‘nsQueryInterfaceWithError’ to ‘nsIAbDirectory*’ in assignment I also don't like that anyway as I think you're passing the aDirectory attribute by value and hence you're just using it as a temporary variable in this case, which isn't good IMO because it can hide what is really going on.

Changes from v0.1k: * Made suggested changes and tested it compiles.

+nsresult nsAbAutoCompleteSession::SearchDirectories(nsIRDFService *aRDFService, ... + // Set rv1 to NS_ERROR_FAILURE. Will be set to NS_OK + // if one of the searches succeeds + nsresult rv1 = NS_ERROR_FAILURE; Could we use something like NS_ERROR_NOT_FOUND? You'd probably have to define this as a new code, but it would enable us to distinguish errors from search failures. +nsresult nsAbAutoCompleteSession::SearchDirectories(nsIRDFService *aRDFService, ... + for (index = 0; index < mAutoCompleteArray.Count(); index++) + { + nsCOMPtr <nsIRDFResource> resource; + rv = aRDFService->GetResource(*mAutoCompleteArray[index], getter_AddRefs(resource)); + NS_ENSURE_SUCCESS(rv, rv); ... + rv = directory->GetDirType(&dirType); + if ((NS_SUCCEEDED(rv) && dirType == 2) || aOffLine) ... + // use the fileName to create the URI for the directory + nsCAutoString URI; + URI = NS_LITERAL_CSTRING("moz-abmdbdirectory://") + fileName; Errm, isn't mAutoCompleteArray[index] now a URI to the directory? Hence you don't need to recalculate it at least for the mdb directory case. Not sure what we'd do about the offline one though. +NS_IMETHODIMP nsAbAutoCompleteSession::GetAutoCompleteArray(PRUint32 *aCount, char ***aArray) ... + if (!aArray) + { + NS_ERROR("nsAbAutoCompleteSession::GetAutoCompleteArray: null pointer "); + return NS_ERROR_NULL_POINTER; + } Use NS_ENSURE_ARG_POINTER here, and on aCount as well. + autoCompleteAddressbooks.AssignLiteral("moz-abmdbdirectory://abook.mab,moz-abmdbdirectory://history.mab"); Use kPersonalAddressbookUri and kCollectedAddressbookUri for the uris please. +function fillInSessionParams(aDirectory, aSession) ... + // get the login to authenticate as, if there is one + var login = ""; + if (directory instanceof Components.interfaces.nsIAbLDAPDirectory) + login = aProperties.authDn; ... + // set the LDAP protocol version correctly + var protocolVersion; + try { + protocolVersion = sPrefs.getCharPref(addressbook + ".protocolVersion"); You can get the protocol version from nsIAbLDAPDirectory.protocolVersion, in the same way (/if statement) as you've done with authDn. + aSession.minStringLength = sPrefs.getIntPref(addressbook + ".autoComplete.minStringLength"); + aSession.cjkMinStringLength = sPrefs.getIntPref(addressbook + ".autoComplete.cjkMinStringLength"); + ldapFormatter.nameFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.nameFormat", + nsISupportsString).data; + ldapFormatter.addressFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.addressFormat", + nsISupportsString).data; + ldapFormatter.commentFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.commentFormat", + nsISupportsString).data; + aSession.outputFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.outputFormat", + nsISupportsString).data; + aSession.filterTemplate = sPrefs.getComplexValue(addressbook + ".autoComplete.filterTemplate", + nsISupportsString).data; I think we could/should expose these through nsIAbLDAPDirectory as well (maybe even nsIAbDirectory, though I'd have to think about that). Though probably should be done in a separate bug. + ldapFormatter.commentFormat = sPrefs.getComplexValue(addressbook + ".description", Should be using nsIAbDirectory.dirName I think. + // XXXdmose should really use .autocomplete.maxHits, + // but there's no UI for that yet + aSession.maxHits = aDirectory.maxHits; That comment can probably be removed now. Note to self: got to certFetchingStatus.js

Final comments for now: Index: mailnews/extensions/smime/resources/content/certFetchingStatus.js ... + if (directory.dirType == 1) { use (directory instanceof Components.classes.nsIAbLDAPDirectory) please - its clearer. I'm currently assuming that the mail/ code changes are pretty similar to the mailnews/ so where I've commented against the mailnews/ they'll get applied to mail/ as well.

Changes since v0.1l: * Made suggested changes * Unbitrotted against trunk * Currently includes Mark's fix from bug 361326

I've just been thinking about this again. I'm not sure I really like mail.autoComplete.addressBooks as a pref. Could we not have a per-addressbook pref (ldap_2.servers.xxx.useForAutoComplete or something)? This would then be a new attribute in nsIAbDirectory. When it comes to finding which servers, we iterate through RDF and check for the attribute. We could also (possibly not in this bug) then very easily expand the UI for setting up each of the address book types so that there is a check box where the user could say "autocomplete this address book". Then there's two places its possible to set things up.

Changes since v0.1o: * Unbitrotted after removal of nsMessengerMigrator * Now using space, instead of comma, as the delimiter between addressbooks * SM prefs now use GetFields functionality to update pref value * Account settings now use onSave functionality to update settings * TB prefs now use onsynctopreferences functionality to update pref value * All pending account setting changes to default identity show up when you go into "Manage Identities" Still to fix: * Investigate why DIR_GetDirectories does not seem to get called when using RDF in pref, account settings and compose windows.

Changes since v0.1r: * Prefs get migrated whenever you go into a compose window, addressing prefs window and account addressing settings.

Unbitrotted version of v0.1s

Comment on attachment 267651 [details] [diff] [review] Work in Progress patch v0.1t Problem scenario: Select two ldap servers that require auth, start autocomplete and get two dialogs, one is filled in, the second is blank. See image I'll attach in a mo. If I then enter invalid password on one, autocomplete comes up with LDAP initialization failed, and goes away, then answering the second one gives a result and LDAP initialization failed - basically it all gets screwed up. Here's some comments I've generated from review the patch so far (note with pref-directory.js I reviewed the new source with the patch applied, so apologies if I've picked up on things you didn't change). - The profile migrators for SM & TB both will need to be updated for the new prefs. - When migrating preferences, I think that if ldap_2.autoComplete.useAddressBooks is true, we should set to autocomplete on all local address books, not just Personal & Collected. We may want to consider clearing the old prefs as well? - 'Edit Directories' button on preferences pane. I think this really needs to be changed to 'Edit LDAP Directories' or something similar. - Possibly put the 'Edit Directories' button underneath the listbox, right justified. - What happens to LDAP sessions (in MsgComposeCommands.js) if we go offline and online again. - Deleting an address book doesn't update the mail.autoComplete.addressBooks pref, this causes a message when you try and autocomplete (below) and in the case of an ldap directory, says LDAP initialization failed.: ERROR: [Exception... "Component returned failure code: 0x804b000a [nsILDAPURL.spec]" nsresult: "0x804b000a (<unknown>)" location: "JS frame :: chrome://messenger/content/messengercompose/MsgComposeCommands.js :: fillInSessionParams :: line 867" data: no] Index: mail/components/compose/content/MsgComposeCommands.js function setupLdapAutocompleteSession() ... + // Add observers to the directory servers we are autocompleting against + // only if observers do not already exist. + // Remove observers from directory servers no longer in the list. This comment looks strange. I suggest adding a blank line between the second and third may clean it up a bit. +function fillInSessionParams(aDirectory, aSession) +{ + var addressbook = aDirectory.dirPrefId; + var serverURL = Components.classes["@mozilla.org/network/ldap-url;1"] + .createInstance() + .QueryInterface(Components.interfaces.nsILDAPURL); + try { + serverURL.spec = aDirectory.URI; + } catch (ex) { + dump("ERROR: " + ex + "\n"); + } + aSession.serverURL = serverURL; What's wrong with aDirectory.lDAPURL? It gives you the serverURL all filled in... + aSession.minStringLength = sPrefs.getIntPref(addressbook + ".autoComplete.minStringLength"); + aSession.cjkMinStringLength = sPrefs.getIntPref(addressbook + ".autoComplete.cjkMinStringLength"); + aSession.outputFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.outputFormat", + aSession.filterTemplate = sPrefs.getComplexValue(addressbook + ".autoComplete.filterTemplate", + ldapFormatter.nameFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.nameFormat", + ldapFormatter.addressFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.addressFormat", + ldapFormatter.commentFormat = sPrefs.getComplexValue(addressbook + ".autoComplete.commentFormat", We probably should expose the first three on in nsIAbDirectory, and the last three in nsIAbLDAPDirectory (separate bug though) - then at least we'd have them documented a bit better ;-). Though I was a little surprised to see them on the addressbook and not global - but its what we do currently, so that's fine. + // override default maxHits (currently 100) + try { + aSession.maxHits = aDirectory.maxHits; + } catch (ex) { + // if this pref isn't there, or is out of range, no big deal. + // just let nsLDAPAutoCompleteSession use its default. + } This seems overkill. maxHits will only fail if we've not got/set the directory right (in which case other parts of this function would fail as well). function setupLdapAutocompleteSession() ... + // use temporary sessions to do the setup so that we don't overwrite the + // global ones, then have some problem and throw an exception, and leave the + // global ones with partially setup sessions. We'll assign the temp sessions + // into the global ones after we're done setting up the sessions + var acSessions = new Object(); + if (gLDAPSessions) + acSessions = gLDAPSessions; I think acSessions = gLDAPSessions; if (!acSessions) acSessions = new Object(); would be better here. +function ReleaseABObservers() +{ + var addressbook; + // Remove observer on all addressbooks since we are not doing autocompletion. + var currentArray = gCurrentAutocompleteList.split(" "); + for (var i in currentArray) { + addressbook = currentArray[i]; + if (addressbookDirType(addressbook) == 1) + RemoveABSettingsObserver(addressbook); + } + gCurrentAutocompleteList = null; } gCurrentAutocompleteList seems to be kept in step with gLDAPSessions. Index: mailnews/mailnews.js ok Index: mailnews/addrbook/prefs/resources/content/pref-addressing.xul ok Index: mailnews/addrbook/prefs/resources/content/pref-directory.js function setupAutocompleteList() ... var acNode = acList.getItemAtIndex(i); acNode.checked = (currentArray.indexOf(acNode.id) > -1); - Skip the temporary and just assign it directly. function saveAutocompleteList(aPageData) - JavaScript strict warning: chrome://messenger/content/addressbook/pref-directory.js, line 73: function saveAutocompleteList does not always return a value function removeFromAutoCompleteList(aList, aBookUri) - contains unused variable deleted. function onInitEditDirectories() ... if (addrbook instanceof nsIAbDirectory && !addrbook.isMailList && addrbook.isRemote) - the last part could do with being on a separate line. function selectDirectory() ... disable = gPrefBranch.getBoolPref(gCurrentDirectoryServerId + ".disable_delete"); - line length again function onAccept() ... // the RDF resource URI for LDAPDirectory will be moz-abldapdirectory://<prefName> - That comment is wrong and too long - Can you just do a general tidy up of line lengths in this function? Index: mailnews/base/public/nsIMsgIdentity.idl ok Index: mailnews/base/util/nsMsgIdentity.cpp ok I know there's more, but I'm doing it a bit at a time. I'll keep working on reviewing, but feel free to update/fix issues as we progress.

Bug 100606 has an (incomplete) patch from 2005 as well.

Does this fix include allowing userss to specify multiple ldap's to be used at the same time for auto-completion? Will this carry over to lightning/calendar for event invitees?

Yes. See also bug 100606.