356208 - Create an nsAbLDAPDirectoryModify class

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

I'm not quite sure what this is going to entail exactly, but this will be the controlling class for modifications (add/modify/delete) on LDAP directories.

I think what we want is two classes:

nsAbModifyLDAPMessageListener - to handle the nsILDAPListener interface that is part of the directory/xpcom code.
nsAbLDAPDirectoryModify - to handle the interface between the Address Book and the LDAP code.

Comment 1

[Jeremy Lainé]

Attachment: Writeable LDAP backend V1

The attached patch (co-written with Mark Banner) provides the backend for writeable LDAP in the addressbook. For now, the code is disabled by default as there is still some UI work to be done. Define MOZ_EXPERIMENTAL_WRITEABLE_LDAP to enable it.

What it adds:

* nsAbLDAPCard : this class now carries some extra member variables to keep track of the card's Distinguished Name (DN) and the attributes the card originally had (set upon retrieving a search result). This is used to determine what LDAP modification need to be done to add / update en entry. Relative Distinguished Name (RDN) building is also done in this class.

* nsAbLDAPDirectory : define the methods need to modify LDAP cards, namely AddCard, DeleteCards and ModifyCard.

* nsAbLDAPDirectoryModify : this class handles actual interaction with the LDAP server, it is modeled after nsAbLDAPDirectoryQuery.

* 2 new preferences (no UI for them yet) :
- "rdnAttributes" :what attributes to use when building the Relative DN for a new card
- "objectClasses" : what LDAP object classes to use for new cards

Comment 2

[Mark Banner (:standard8)]

David, Scott: Jeremy and I have been working on this patch together for a while now. I've reviewed it myself several times, but its not right for me to do it officially as I've done some of it myself, and probably the more eyes we get on it the better ;-)

As Jeremy mentioned in comment 1 it does the necessary backend work to get writeable ldap directories. We're still missing a significant amount of the UI work - mainly async capable dialogs - the success/failure is just logged via printfs at the moment and we're missing most of the callback functions which would do the UI stage.

There's also a lot of XXX statements that we'll need to fix as we get the UI calls in place and generally progress it.

So the writeable ldap code will be disabled via the nsAbLDAPDirectory::Operations function as per the define MOZ_EXPERIMENTAL_WRITEABLE_LDAP, the rest of the code will be compiled, just not accessible for nightlies (I think this is the best way as it avoids loads of ifdefs, whilst also allowing breakages to be quickly rectified).

This will allow us to work on getting the UI half decent. Once the basic UI is working, then we'll probably allow it to be enabled via a pref, and hence accessible to nightly testers. Once its then had some more testing/fixes we'll enable it fully, but that's a little way off yet.

Comment 3

[David :Bienvenu]

Comment on attachment 281949 [details] [diff] [review]
Writeable LDAP backend V1

This is very cool. Some minor comments, sr=bienvenu with the comments addressed however you see fit, Mark.

Do you want to leave the printfs in for now? Or use logging?
nsCAutoString and nsCString are the same thing on the trunk, so we've just been using nsCString...

the braces style is inconsistent - I hesitate to point that out since I prefer 

if (a)
{
  ...
}

but I think Mark might be a K&R guy :-)

Minor nit - There's some inconsistency in arg naming, i.e., do args start with "a" or not. E.g.,

  nsAbModifyLDAPMessageListener(nsIAbLDAPDirectory *abDir,
+                                const PRInt32 type,
+                                const nsACString &aCardDN,
+                                nsIArray* modArray,
+                                const nsACString &aNewRDN,
+                                const nsACString &aNewBaseDN,
+                                nsILDAPURL* url,
+                                nsILDAPConnection* connection,
+                                const nsACString &login,
+                                const PRInt32 timeOut = 0);


Generally, we tend to just use the aArg convention for interface method declarations and implementations.

+  // XXX we need to fix how this all works - specifically, see the first patch
+  // on bug 124553 for how the quey equivalent did this
+  // too soon? Do we need a new listener?

"query"?

+  if (_messageListener == NULL)
+    return NS_ERROR_OUT_OF_MEMORY;
+  //  mListener = _messageListener;
+  //    *_retval = 1;

remove the commented out lines?

The new copyrights say 2006 - should be 2007?

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> (From update of attachment 281949 [details] [diff] [review])
> Do you want to leave the printfs in for now? Or use logging?

I'm happy to leave them in for now as a) although the code is built, its disabled, b) we'll have a better solution at the end (i.e. a UI) for most of them, and the others will need to be converted.

> nsCAutoString and nsCString are the same thing on the trunk, so we've just been
> using nsCString...

ok

> the braces style is inconsistent - I hesitate to point that out since I prefer 

Opps, missed that. Some of them are indented wrongly as well.

For c++ code I suggest:

if (...)
{
  ...
}

for js:

if (...) {
  ...
}

I think that is the general style the address book is coming out with.

> Minor nit - There's some inconsistency in arg naming, i.e., do args start with
> "a" or not. E.g.,
> 
>   nsAbModifyLDAPMessageListener(nsIAbLDAPDirectory *abDir,
> +                                const PRInt32 type,
> +                                const nsACString &aCardDN,
> +                                nsIArray* modArray,
> +                                const nsACString &aNewRDN,
> +                                const nsACString &aNewBaseDN,
> +                                nsILDAPURL* url,
> +                                nsILDAPConnection* connection,
> +                                const nsACString &login,
> +                                const PRInt32 timeOut = 0);
> 
> 
> Generally, we tend to just use the aArg convention for interface method
> declarations and implementations.

Yes, we should make that consistent, though some of it is probably my fault.

> +  // XXX we need to fix how this all works - specifically, see the first patch
> +  // on bug 124553 for how the quey equivalent did this
> +  // too soon? Do we need a new listener?
> 
> "query"?

Yep.

> +  if (_messageListener == NULL)
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  //  mListener = _messageListener;
> +  //    *_retval = 1;
> 
> remove the commented out lines?

Yes I think we should do.

> The new copyrights say 2006 - should be 2007?

Yep, don't think I wrote it back in 2006 ;-)

Jeremy, do you want to update this or shall I?

Comment 5

[Jeremy Lainé]

I'll update the patch and post it here when done.

Comment 6

[Jeremy Lainé]

Attachment: Writeable LDAP backend V2

Attached is an update patch for the editable LDAP backend.

Changed:
* make use of curly braces consistent (+ fix 1 indentation problem)
* update copyright notices
* make arguments in nsAbLDAPDirectoryModify.cpp consistent with nsAbLDAPDirectoryQuery.cpp 
* remove some commented code
* fix typo "quey" => "query"

Not changed:
* the strings on the stack are kept as nsCAutoString rather than nsCString (result of an IRC chat with dmose)

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 282228 [details] [diff] [review]
Writeable LDAP backend V2

I've had a quick look at this and this seems a lot better.

Carrying forward David's r, requesting sr.

Comment 8

[Scott MacGregor]

Comment on attachment 282228 [details] [diff] [review]
Writeable LDAP backend V2

bump the IID for nsIAbLDAPAttributeMap.idl.

can this:

+  NS_ADDREF(*aLDAPAddMessageInfo = modArray);

be replaced with
modArray.swap(*aLDAPAddMessageInfo); ?

nice work guys.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 282228 [details] [diff] [review])
> can this:
> 
> +  NS_ADDREF(*aLDAPAddMessageInfo = modArray);
> 
> be replaced with
> modArray.swap(*aLDAPAddMessageInfo); ?

It could, but we'd have to QI modArray to a nsIArray (its currently a nsIMutableArray), so given that, I think it probably will be about the same speed wise either way. I've left it as NS_ADDREF for now as that's probably smaller code size.

I've just updated the patch for the IID and checked it in.

Comment 10

[Mark Banner (:standard8)]

This is now checked in with a couple of bustage fixes. As we've now got a nsAbLDAPDirectoryModify class, I'm going to declare this as fixed and we'll deal with the UI and other modifications/fixes in separate bugs.

Thanks to Jeremy for taking on my original patch and helping push it through.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #9)
>
> It could, but we'd have to QI modArray to a nsIArray (its currently a
> nsIMutableArray), so given that, I think it probably will be about the same
> speed wise either way. 

Given that nsIMutableArray inherits from nsIArray, I wouldn't expect a QI to be necessary there, FWIW.