Closed Bug 124057 Opened 18 years ago Closed 18 years ago

when I delete addressbooks from the prefs (addressing panel) it doesn't get removed from the directory pane (until I restart)

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

x86
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: sspitzer, Assigned: srilatha)

References

()

Details

(Whiteboard: nab-ldap,[ADT3])

Attachments

(1 file, 3 obsolete files)

when I delete addressbooks from the prefs (addressing panel) it doesn't get
removed from the directory pane.

we're going to have to poke the directory datasource.  we'll have to make sure
we handle the case where you delete the one you've got selected in the
addressbook.  we should already be handling this scenario in the addressbook
sidebar.

we should check with jglick if she wants it to disappear from just autocomplete,
but remain in the addressbook (I don't think she will, but we should check.)
Whiteboard: nab-ldap
it fixes on restart.
Summary: when I delete addressbooks from the prefs (addressing panel) it doesn't get removed from the directory pane → when I delete addressbooks from the prefs (addressing panel) it doesn't get removed from the directory pane (until I restart)
reassign QA contact to myself
QA Contact: nbaca → yulian
Agree. If a user removes a directory using either the Prefs: LDAP Directory 
Servers dialog Or the Address Book window, the directory should be removed from 
Prefs for autocomplete and the Address Book window.

(An alternative we can explore if folks are interested is here: 
http://www.mozilla.org/mailnews/specs/addressbook/images/LdapSel2.gif
http://www.mozilla.org/mailnews/specs/addressbook/#Prefs)
over to srilatha, she's working on making it so "add" from the autocomplete UI 
shows up (without restart) and making delete work the same may (and modify) is 
very related.
Assignee: sspitzer → srilatha
Keywords: nsbeta1
Target Milestone: --- → mozilla0.9.9
Keywords: nsbeta1nsbeta1+
Priority: -- → P2
20020218 builds

It doesn't fix on restart for Win2K &Wins NT.

Fine with MacOS 9.2 if restart.
The problem it doesn't fix on restart for Win2K & Wins NT is turbo mode.
Without turbo mode, it fixes on restart.

Moving to 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Attached patch patch v1 (obsolete) — Splinter Review
Deleting a directory from the preferences window now calls
deleteAddressBookPrefs. This a new method I added to nsIAddressBook. This is
same as deleteAddressBooks  except the first parameter.
void deleteAddressBooks(in nsIRDFCompositeDataSource db, in nsISupportsArray
parentDir, in nsISupportsArray aResourceArray);
+  void deleteAddressBookPrefs(in nsIRDFDataSource db, in nsISupportsArray
parentDir, in nsISupportsArray aResourceArray);
With this patch deleting a directory now goes through the nsDirPrefs code
Status: NEW → ASSIGNED
+      // the rdf service
+      var RDF = '@mozilla.org/rdf/rdf-service;1'
+      RDF = Components.classes[RDF].getService();
+      RDF = RDF.QueryInterface(Components.interfaces.nsIRDFService);
+      // get the datasource for the addressdirectory
+      var addressbookDS = RDF.GetDataSource("rdf:addressdirectory");
+      var addressbook =
Components.classes["@mozilla.org/addressbook;1"].createInstance();
+      addressbook =
addressbook.QueryInterface(Components.interfaces.nsIAddressBook);
+

Shouldnt the above code be in a try - catch block.

Same with the 2nd hunk in the patch for this file.

The code that calls the nsDirPrefs and removes the dir entry from the UI should
be here. Srilatha told me that it is part of patch in bug # 124059, please make
sure the below hunk is part of this patch :

NS_IMETHODIMP nsAbBSDirectory::DeleteDirectory(nsIAbDirectory *directory)
{
	nsresult rv = NS_OK;
	
	if (!directory)
		return NS_ERROR_NULL_POINTER;
+    
+    // if addressbook is not launched yet mSevers will not be initialized
+    // callin GetChidNodes will initialize mServers
+    nsCOMPtr<nsIEnumerator> subDirectories;
+    rv = GetChildNodes(getter_AddRefs(subDirectories));
+    NS_ENSURE_SUCCESS(rv, rv);

	DIR_Server *server = nsnull;
	nsVoidKey key((void *)directory);
	server = (DIR_Server* )mServers.Get (&key);


For the above code, since GetChildNodes should be called only if mServers is not
initialized, check the mInitialized member before making this call to avoid
unnecessary call if not required.


Also Srilatha explained me that in case if delete is done from prefs instead of
directly accessing nsDirPrefs from the C++ function DeleteAddressBookPrefs() in
the patch (id=73918), she needs to go through the RDFResource impl for this
(nsABSDirectory) since it is possible that the Address Book window is open at
that time and it should be updated. Please document this in your code, maybe in
function DeleteAddressBookPrefs().


Discussed in Mail News bug mtg with Engineering QA Mktng PjM.  Decided to ADT3
this bug. 
Whiteboard: nab-ldap → nab-ldap,[ADT3]
Attached patch patchv2 (obsolete) — Splinter Review
Attachment #73918 - Attachment is obsolete: true
20020402 trunk build on WIn2k
It fixes on restart for Wins in turbo mode (&non turbo mode).
20020415 1.0 branch build
In turbo mode,
deleting LDAP Directory in Prefs, the Directory Server still shows up in Address
book window, addressbook Sidebar and Select Address dlg after restart.
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.
Keywords: nsbeta1-
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.
Keywords: nsbeta1+
+        // the rdf service
+        var RDF = '@mozilla.org/rdf/rdf-service;1'
+        RDF = Components.classes[RDF].getService();
+        RDF = RDF.QueryInterface(Components.interfaces.nsIRDFService);

instead of using the same RDF var repeatedly maybe you can combine the above 
three lines into one statement.

Also please add comments for function DeleteAddressBookPrefs() as I mentioned 
above in comment # 10.

do the above and you have r=rdayal.
Attached patch updated patch. (obsolete) — Splinter Review
updated the patch based on rajiv's comments.
Attachment #75601 - Attachment is obsolete: true
Comment on attachment 82856 [details] [diff] [review]
updated patch.

r=rdayal based on comment #17
Attachment #82856 - Flags: review+
Looks good.  Comments:

1)  

maybe it's my C / C++ upbringing, but I'd rather you declare var RDF and var
addressbook in the proper scope.
I know it works from JS the way you did it, but it makes the code harder to
read.  You declare them in the first
try block, and use them later.

2)

+        var addressbook =
Components.classes["@mozilla.org/addressbook;1"].createInstance();
+        addressbook =
addressbook.QueryInterface(Components.interfaces.nsIAddressBook);

Why not:

+        var addressbook =
Components.classes["@mozilla.org/addressbook;1"].createInstance(Components.interfaces.nsIAddressBook);

3)

+      catch(ex){}

since we don't expect failure, I'd add a dump statement to your two catch
blocks.  Failing silently makes it hard to debug.

4)

+          var parentDir =
RDF.GetResource("moz-abdirectory://").QueryInterface(Components.interfaces.nsIAbDirectory);

can you add a comment about how "moz-abdirectory://" is the special URI for the
boot strap directory?

5)

+          var selectedABURI = "moz-abldapdirectory://" + gDeletedDirectories[i];

can you add a comment about how the resource for a LDAP directory is that schema
+ the prefname?

6)

   void deleteAddressBooks(in nsIRDFCompositeDataSource db, in nsISupportsArray
parentDir, in nsISupportsArray aResourceArray);
+  void deleteAddressBookPrefs(in nsIRDFDataSource db, in nsISupportsArray
parentDir, in nsISupportsArray aResourceArray);

The implementations for deleteAddressBooks and deleteAddressBookPrefs look very
similar.  What if you just changed
deleteAddressBooks() to take a nsIRDFDataSource instead of a
nsIRDFCompositeDataSource?  You might have to add a QI in there.
I think it's possible to not add this new method.

About the new method, I don't think having prefs in the name is correct.
That's an implementation detail.  (that we use prefs as how we persist
addressbooks.)  That implementation detail should not be part of our interface.

7)

It looks like you copied the existing bad style.  But as a style issue, I like
the aFoo notation for arguments.
It makes code easier to read.  If you are able to combine deleteAddressBooks()
and deleteAddressBooksPrefs(),
consider switching all the arguments to the aFoo style.
updated the patch based on Seth's comments. In this patch we are not using
DeleteAddressbookPrefs but I changed the parameter of DeleteAddressBooks.
Attachment #82856 - Attachment is obsolete: true
Comment on attachment 83125 [details] [diff] [review]
patch addressing Seth's comments

sr=sspitzer

one comment:

+      catch(ex){
+	 dump("Failed to get RDF Service or addressbook \n");
+      }

consider:

+      catch(ex){
+	 dump("Failed to get RDF Service or addressbook: " + ex + "\n");
+      }

That way, we'll know what the exception was.  No need for a new patch.
Attachment #83125 - Flags: superreview+
Comment on attachment 83125 [details] [diff] [review]
patch addressing Seth's comments

carrying over r=rdayal
Attachment #83125 - Flags: review+
Fix checked in on 5/10
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
20020517 trunk builds

Works for MacOS X but Deleting Directory fails on WIn2k...please refer to
regression Bug 145346.
20020529  trunk build on Win2K

Verified it get removed directory pane.
Status: RESOLVED → VERIFIED
Please have the fix checked in Branch build. 
Changing nsbeta1- to nsbeta1, nominate for Buffy.
Keywords: nsbeta1-nsbeta1
Removed keyword, nsbeta1. The fix doesn't need to be checked into branch build.
Keywords: nsbeta1
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.