when I add LDAP directories from the prefs (addressing panel) it doesn't get added to the addressbook (until I restart). we're going to have to poke the directory datasource.

Assign QA contact to myself.

over to srilatha, she is working on related issues. see AbDeleteDirectory() in addressbook.js note, you won't have to do as much for LDAP delete (except maybe some locked pref checks?). there is extra code in there to prompt you and to prevent you from deleting your CAB or PAB. note, unlike new addressbook, delete actually does do a command. see nsAddressBook::DeleteAddressBooks()

20020218 builds On Win2k & Wins NT, LDAP directory added from Prefs doesn't get added to the addressbook even restart. MacOS 9.2 is fine if restart.

It fixes on restart on Win2k & Wins NT if turning off turbo mode.

Is bug 126265 a dupe of this?

Yes, Srilatha, would you mark bug 126265 dupe of this?

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

Discussed in 2/27/02 Mail & News bug meeting. Decisions was to plus this bug as a P2.

Turbo mode is default to users. In other words, most of them won't be able to see the LDAP Directory added from Prefs in Address Book even restart unless they're aware of the workaround to turn off turbo mode and then restart.

This patch is taken from the original bug #83091 patch. I have updated it against the trunk. It supports adding, editing and removing LDAP Directory Servers. It might be useful as a reference. This patch would be good for bug #124061, bug #124057 and bug #124007. I will polish it up.

With this patch when a directory is added we go through the nsDirPrefs to add the preferences. Added three new attributes to nsIAbDirProperties + attribute long dirType; // 0 for ldap, 2 for addressbooks + attribute long maxHits; + attribute boolean authEnabled; // not being used yet. will need once we add secure and logon checkboxes (bug 120432)

The above patch is only for adding a directory. Posting a patch for delete in 124057

updated the patch with the latest trunk. The addressbook-panel.js file has changed from the time I created the previous patch(attach 74413). I found some problems because of these changes. When a LDAP directory is selected in the sidebar panel, I see some javascript errors because ResultsPaneSelectionChanged() of abSelectAddressesDialog.js is getting called instead of the the one in addressbook-panel.js from ChangeDirectoryByDOMNode(). To fix this I moved some code out of abSelectAddressesDialog.js to abSearch.js(new file).

Discussed in Mail News bug mtg with Engineering QA Mktng PjM. Decided to ADT3 this bug.

Srilatha, Is the fix going to be checked in soon?

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Updated the patch against the lates trunk

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

Nominate this bug for Buffy. Keep Keyword: nsbeta1

Related to bug 124057 - when I delete addressbooks from the prefs (addressing panel) it doesn't get removed from the directory pane (until I restart).

We have a patch here which affects usability in a very positive way. Can we get it reviewed and landed in time for testing? Thanks.

reassigning to shliang. Shuehan, can you drive this in to the trunk?

taking. I'm going to work on driving this into the trunk, while shuehan focuses on browser.

over to cavin

I haven't closely reviewed, but after taking a quick look, the most recent patch from srilatha looks pretty good. I fear it might have suffered some bit rot. cavin, can you start by applying it, and getting it to work again with a fresh tree? this blocks bug #17230

The most recent patch from srilatha does work fine fine with the trunk code. It also works for renaming normal address books.

The new function AbRenameAddressBook() in addressbook.js is to be used by the rename addrbook UI code when it is ready (bug 17230).

Comment on attachment 114920 [details] [diff] [review] Updated patch against the trubk Reset. I need to add some more stuff from but 17230 to here.

Includes original patch and part of the patch (Get/SetDirPrefId()) from 17230.

Comment on attachment 115665 [details] [diff] [review] Proposed patch, v1 comments: 1) can you make 0 a constant? 0 is really LDAPDirectory, from nsDirPrefs.h, right? + properties.dirType = 0; 2) why make modifyAddressBook() take isupports arrays? I see that deleteAddressBooks() does this. Were you just following that? there's doesn't appear to be any good reason, except that the back end will need it before calling ::DoCommand() can you fix it so modifyAddressBook() takes the actual args: nsIAbDirectory parentDir, nsIABDirectory directory, nsIAbDirectoryProperties and then in ::ModifyAddressBook(), do the work to converting to nsISupports arrays We should fix deleteAddressBooks(), too. you'll have to fix the callers of modifyAddressBook() and deleteAddressBooks() + if (gCurrentDirectory && gCurrentDirectoryString) { + // we are modifying an existing directory + // the rdf service + var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"]. + getService(Components.interfaces.nsIRDFService); + // get the datasource for the addressdirectory + var addressbookDS = RDF.GetDataSource("rdf:addressdirectory"); + var parentArray = Components.classes["@mozilla.org/supports-array;1"].createInstance(Components.i nterfaces.nsISupportsArray); + + // moz-abdirectory:// is the RDF root to get all types of addressbooks. + var parentDir = RDF.GetResource("moz-abdirectory://").QueryInterface(Components.interfaces.nsIA bDirectory); + parentArray.AppendElement(parentDir); + var resourceElement = Components.classes["@mozilla.org/supports-array;1"].createInstance(Components.i nterfaces.nsISupportsArray); + + // the RDF resource URI for LDAPDirectory will be moz-abldapdirectory://<prefName> + var selectedABURI = "moz-abldapdirectory://" + gCurrentDirectoryString; + var selectedABDirectory = RDF.GetResource(selectedABURI).QueryInterface(Components.interfaces.nsIAbDirect ory); + var selectedABResource = selectedABDirectory.QueryInterface(Components.interfaces.nsIRDFResource); + + // resourceArray is an array of resourceElements. + // ResourcesElement is an array, where the first element is nsIAbDirectory + // and the second element is nsIAbDirectoryProperties + resourceElement.AppendElement(selectedABResource); + resourceElement.AppendElement(properties); + var resourceArray = Components.classes["@mozilla.org/supports-array;1"].createInstance(Components.i nterfaces.nsISupportsArray); + resourceArray.AppendElement(resourceElement); + addressbook.modifyAddressBook(addressbookDS, parentArray, resourceArray); + window.opener.gNewServerString = gCurrentDirectoryString; void newAddressBook(in nsIAbDirectoryProperties aProperties); + void modifyAddressBook(in nsIRDFDataSource aDS, in nsISupportsArray aParentDir, in nsISupportsArray aResourceArray); void deleteAddressBooks(in nsIRDFDataSource aDS, in nsISupportsArray aParentDir, in nsISupportsArray aResourceArray); void setDocShellWindow(in nsIDOMWindowInternal win); void exportAddressBook(in nsIAbDirectory aDirectory); 3) GetABView().directory and shuehan onItemPropertyChanged: function(item, property, oldValue, newValue) { - // will not be called + try { + var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory); + // check if the item being changed is the directory + // that we are showing in the addressbook sidebar + if (directory == GetAbView().directory) { + LoadPreviouslySelectedAB(); + } + } + catch (ex) { + } 4) can you make a const for this? + properties.dirType = 2; 2 is really PABDirectory, from nsDirPrefs.h, right? 5) there's a bug about adding support for this to the UI, right? can you include the bug # in this code and make a note in that bug about this code? +// not used yet. just here for when we fix bug #xxxxxx +function AbRenameAddressBook() +{ 6) + rv=aProperties->GetAuthDn(getter_Copies(authDn)); nit, should be + rv = aProperties->GetAuthDn(getter_Copies(authDn)); 7) + nsresult rv = NS_OK; nit, no need to initalize rv. 8) + if (!directory) + return NS_ERROR_NULL_POINTER; + if (!aProperties) + return NS_ERROR_NULL_POINTER; use NS_ENSURE_ARG_POINTER() 9) + // calling GetChidNodes will initialize mServers typo, GetChildNodes() 10) + // Save modified address book into pref file. + nsCOMPtr<nsIPref> pPref(do_GetService(NS_PREF_CONTRACTID, &rv)); + NS_ENSURE_SUCCESS(rv, rv); + return pPref->SavePrefFile(nsnull); this is an obsolete interface. See nsIPrefService and nsIPrefBranch for the new implementations. 11) +NS_IMETHODIMP nsAbOutlookDirectory::ModifyDirectory(nsIAbDirectory *directory, nsIAbDirectoryProperties *aProperties) +{ + return NS_OK; +} this should return NS_ERROR_NOT_IMPLEMENTED, right? 12) why is this a for loop? I think parentDirs is always just an XPCOM array with one element, right? if so, remove the loop and just ensure that itemCount == 1. (And assert and return error if not) also, I suggest adding code that checks that resourceArray count is always 2. also, use do_QueryElementAt() to make your code simpler and cleaner. +nsresult nsAbDirectoryDataSource::DoModifyDirectory(nsISupportsArray *parentDirs, nsISupportsArray *arguments) +{ + PRUint32 item, itemCount; + nsresult rv = parentDirs->Count(&itemCount); + NS_ENSURE_SUCCESS(rv, rv); + + nsCOMPtr<nsISupportsArray> dirArray; + NS_NewISupportsArray(getter_AddRefs(dirArray)); + + for (item = 0; item < itemCount; item++) + { + nsCOMPtr<nsISupports> supports = getter_AddRefs(parentDirs->ElementAt(item)); + nsCOMPtr<nsIAbDirectory> parent = do_QueryInterface(supports, &rv); + if (NS_SUCCEEDED(rv)) + { + // each element in the arguments list is an array of two elements + // the first one is nsIAbDirectory + // the second one is nsIAbDirectoryProperties. + supports = getter_AddRefs(arguments->ElementAt(item)); + nsCOMPtr<nsISupportsArray> resourceArray = do_QueryInterface(supports, &rv); + NS_ENSURE_SUCCESS(rv, rv); + supports = getter_AddRefs(resourceArray->ElementAt(0)); + nsCOMPtr<nsIAbDirectory> modifiedDir(do_QueryInterface(supports, &rv)); + NS_ENSURE_SUCCESS(rv, rv); + supports = getter_AddRefs(resourceArray->ElementAt(1)); + nsCOMPtr<nsIAbDirectoryProperties> properties(do_QueryInterface(supports, &rv)); + NS_ENSURE_SUCCESS(rv, rv); + + if (modifiedDir && properties) + { + rv = parent->ModifyDirectory(modifiedDir, properties); + } + } + } + return rv; }

I didn't get to finish this comments: GetABView().directory and shuehan onItemPropertyChanged: function(item, property, oldValue, newValue) { - // will not be called + try { + var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory); + // check if the item being changed is the directory + // that we are showing in the addressbook sidebar + if (directory == GetAbView().directory) { + LoadPreviouslySelectedAB(); + } + } + catch (ex) { + } shuehan has a patch (she might have checked in already?) that deals with some of the issues with using GetSelectedDirectory() and GetAbView().directory we should run this by her.

Incorporated comments.

Comment on attachment 116114 [details] [diff] [review] Proposed patch, v2 Shuehan, can you also check Seth's comment #34? Thanks.

The bug for cleaning up deleteAddressBooks() is 195493.

Comment on attachment 116114 [details] [diff] [review] Proposed patch, v2 r/sr=sspitzer, nice work cavin just a few small nits to verify (or fix) in your local tree before you check in. 1) + try { + var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory); + // check if the item being changed is the directory + // that we are showing in the addressbook sidebar + if (directory == GetAbView().directory) { + LoadPreviouslySelectedAB(); + } + } + catch (ex) { + } what is the value of GetAbView().directory if you do a quick search in sidebar? will it be the nsIAbDirectory for the quick search, or for the "real" directory? 2) - nsresult rv = NS_OK; + nsresult rv; - if (!directory) - return NS_ERROR_NULL_POINTER; + NS_ENSURE_ARG_POINTER(directory); // if addressbook is not launched yet mSevers will not be initialized - // calling GetChidNodes will initialize mServers + // calling GetChildNodes will initialize mServers if (!mInitialized) { nsCOMPtr<nsIEnumerator> subDirectories; rv = GetChildNodes(getter_AddRefs(subDirectories)); I can't tell from this patch, but I'm assuming that rv will get set. 3) + GetDirPrefId(prefId); + server->prefName = nsCRT::strdup(prefId.get()); + DIR_GetPrefsForOneServer(server, PR_FALSE, PR_FALSE); you should assert and return failure if GetDirPrefId() fails or if prefId is empty. 4) I suggest: + if (itemCount != 1) + { + NS_ASSERTION(PR_FALSE, "DoModifyDirectory() must have parent directory count = 1."); + return NS_ERROR_FAILURE; + } NS_ASSERTION(itemCount == 1, "DoModifyDirectory() must have parent directory count = 1."); if (itemCount != 1) return NS_ERROR_FAILURE; same here: + if (itemCount != 2) + { + NS_ASSERTION(PR_FALSE, "DoModifyDirectory() must have resource argument count = 2."); + return NS_ERROR_FAILURE; + } 5) + nsresult DoModifyDirectory(nsISupportsArray *parentDirs, + nsISupportsArray *arguments); parentDirs should be parentDir, right?

Fix checked in.

Trunk build 2003-03-05: Mac 10.1.5, WinXP Verified Fixed. Looks great!

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

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

this is a wrong bug :)