331557 - Remove nsIAbDirectoryProperties

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

nsIAbDirectoryProperties is a interface that doesn't really support preferences very well and needs replacing by preferences options within nsIAb*Directory.

Removing this should also help remove some of our dependencies on nsDirPrefs

Comment 1

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (normal patch)

This patch removes the c++ uses of GetDirectoryProperties from the address book and related code. At this stage I'm keeping the new pref accessor methods as read-only until I figure out what needs to be done when each of the prefs is updated and what effect that will have on the rest of the system.

This patch includes the requested sr changes from bug 323181.

I'd also like to change the return types of the string prefs to ACString, but decided that was a bit much for this patch.

I'll ask David to check compilation on Windows & with Palm Sync when I ask for sr.

Comment 2

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (diff -w)

Opps, I got the diffs the wrong way round, this is the real -w.

Comment 3

[[e-mail]]

Comment on attachment 216246 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (diff -w)

>+  // XXX These are readonly for now, until we get nsIAbDirectoryProperties
>+  // eliminanted a bit more.
eliminated (only one 'n').

>+  readonly attribute unsigned long dirType;
Would you mind making this a (signed) long?

>+  // XXX to do, convert Get*StringValue & users to use nsACString for result
Until then, would it be possible to leave the new nsIAbDirectory properties as string type (possibly excluding GetURI)?

>+  if ((const PRUnichar*)wvalue)
Please use .IsEmpty(), .IsVoid() or .get() == null as appropriate.

>+    NS_ConvertUTF16toUTF8 utf8str(wvalue.get());
No need to .get() here, but...

>+    *aResult = ToNewCString(utf8str);
I did some more digging and discovered ToNewUTF8String :-)

>+  nsresult rv = GetStringValue("uri", "", getter_Copies(result));
>+
>+  if (NS_FAILED(rv) || result.IsEmpty())
NS_FAILED(rv) guarantees result.IsEmpty() due to COM rules on out pointers.

>   nsXPIDLCString uri;
>-  rv = dirProperties->GetURI(getter_Copies(uri));
>+  rv = abDirectory->GetURI(uri);
nsCAutoString or nsCString uri;

Comment 4

[[e-mail]]

Comment on attachment 216245 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (normal patch)

See comments on -w version.

Comment 5

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v2 diff -w)

Updated patch addressing Neil's review comments. Note I also changed PalmSyncTimeStamp to unsigned long (from long) after discussion on IRC with David, and confirmation that we should be passing it around address book as an unsigned value.

Comment 6

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v2 normal)

Comment 7

[[e-mail]]

Comment on attachment 216426 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v2 diff -w)

>+  if (wvalue.IsEmpty())
>+  {
>+    *aResult = ToNewUTF8String(wvalue, nsnull);
nsnull is the default, you can omit it here.

>+  }
>+  else
>+    *aResult = aDefaultValue ? nsCRT::strdup(aDefaultValue) : nsnull;
>+
>+  return rv;
Sorry I overlooked this. Under XPCOM rules we're not allowed to return both a failure code and a value. I guess this should always return NS_OK ...

>+  nsresult rv = GetStringValue("uri", "", getter_Copies(result));
>+
>+  if (NS_FAILED(rv))
... so you will want to replace these with IsEmpty checks instead, which is what I had intended all along.

r=me with these fixed.

Comment 8

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v3 diff -w)

There's been a few discussions on irc about the GetLocalizedStringValue function, and I'm a bit lost now, I think this is what's needed, so requesting review again to make sure.

Comment 9

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v3 normal)

Comment 10

[[e-mail]]

Comment on attachment 218407 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v3 diff -w)

>+  if (NS_SUCCEEDED(rv))
>+  {
>+    rv = locStr->ToString(getter_Copies(wvalue));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
This change is wrong, v2 was correct.

>+  nsresult rv = GetStringValue("uri", "", getter_Copies(result));
>+
>+  if (NS_FAILED(rv))
These still need to be changed to use result.IsEmpty() instead, as GetStringValue now always returns NS_OK.

Comment 11

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v4 diff -w)

Updated version, only changing NS_FAILED to IsEmpty as per discussion with Neil on IRC.

Comment 12

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v4 normal)

Comment 13

[[e-mail]]

Comment on attachment 220319 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v4 diff -w)

We're agreed at last ;-)

Comment 14

[David :Bienvenu]

Comment on attachment 220319 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v4 diff -w)

+
+  if (wvalue.IsEmpty())
+  {
+    *aResult = ToNewUTF8String(wvalue);
+  }
+  else
+    *aResult = aDefaultValue ? nsCRT::strdup(aDefaultValue) : nsnull;
+


should this be if (!wvalue.IsEmpty()) ?

Comment 15

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v5 diff -w) (checked in)

David is right - that if was the wrong way round :/ Here's the corrected version.

Comment 16

[Mark Banner (:standard8)]

Attachment: Remove c++ uses of GetDirectoryProperties (v5 normal) (checked in)

Comment 17

[David :Bienvenu]

Comment on attachment 220375 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v5 diff -w) (checked in)

thx, Mark, sr=bienvenu, but you don't need these extra braces here:

+  else
+  {
+    *aResult = ToNewUTF8String(wvalue);
+  }
+

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 220375 [details] [diff] [review]
Remove c++ uses of GetDirectoryProperties (v5 diff -w) (checked in)

Checked in with PalmSync change from previous patch - sorry I'd missed including that in this version.

Comment 19

[Mark Banner (:standard8)]

Attachment: Convert pref functions to use ACString for values.

This is something I was going to do in the first patch, but left it to this one as it'd have been too much. The basic idea here is to use the ACString parameters in idl files rather than string. This should give us more readable code and less explicit copying (it also reduces codesize slightly on debug builds).

David, can you check this compiles on windows as well please? I think the palmsync/outlook modifications are correct but I'd just like to check.

Comment 20

[David :Bienvenu]

will do.

Comment 21

[David :Bienvenu]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAbOutlookDirectory.cpp&branch=&root=/cvsroot&subdir=mozilla/mailnews/addrbook/src&command=DIFF_FRAMESET&rev1=1.24&rev2=1.25

I had to check this in as well to fix windows build bustage.

Comment 22

[Mark Banner (:standard8)]

Attachment: Convert pref functions to use ACString for values v2

Updated patch due to bitrot.

Comment 23

[David :Bienvenu]

Comment on attachment 221354 [details] [diff] [review]
Convert pref functions to use ACString for values v2

r=bienvenu, except, I had to make nsAbDirProperty::GetStringValue look like this to get it to compile on Windows:

  nsXPIDLCString value;

  /* unfortunately, there may be some prefs out there with "(null)" values */
  if (NS_SUCCEEDED(m_DirectoryPrefs->GetCharPref(aName, getter_Copies(value))) &&
        !value.EqualsLiteral("(null)"))
    aResult = value;
  else
    aResult = aDefaultValue;

  return NS_OK;

Comment 24

[Mark Banner (:standard8)]

Attachment: Convert pref functions to use ACString for values v3 (checked in)

Fixed the problem with compilation that David picked up (thanks David). Carrying forward his r. Requesting sr.

Comment 25

[[e-mail]]

Comment on attachment 221470 [details] [diff] [review]
Convert pref functions to use ACString for values v3 (checked in)

>+  ACString getLocalizedStringValue(in string aName, in ACString aDefaultValue);
Since you convert UTF16 to UTF8 below I guess these should be AUTF8String

>-  rv = GetFileName(getter_Copies(prefStringValue));
>+  rv = GetFileName(prefStringValue);
>   NS_ENSURE_SUCCESS(rv, rv);
>   
>-  rv = properties->SetFileName(prefStringValue);
>+  rv = properties->SetFileName(prefStringValue.get());
>   NS_ENSURE_SUCCESS(rv, rv);
I assume none of these are expecting null strings, right?

>+    aResult = NS_ConvertUTF16toUTF8(wvalue).get();
Should use CopyUTF16toUTF8

>+  return m_DirectoryPrefs->SetCharPref(aName, nsPromiseFlatCString(aValue).get());
Should be PromiseFlatCString without the ns

> NS_IMETHODIMP nsAbLDAPDirectory::GetURI(nsACString &aURI)
> {
>-  // XXX to do, convert GetStringValue & users to use nsACString for result
>-  nsXPIDLCString result;
>-  nsresult rv = GetStringValue("uri", "", getter_Copies(result));
>+  nsCAutoString result;
>+  nsresult rv = GetStringValue("uri", EmptyCString(), result);
You don't need the temporary, just use aURI directly. [Why is this code duplicated for each type of addressbook?]

Comment 26

[Mark Banner (:standard8)]

(In reply to comment #25)
> >-  // XXX to do, convert GetStringValue & users to use nsACString for result
> >-  nsXPIDLCString result;
> >-  nsresult rv = GetStringValue("uri", "", getter_Copies(result));
> >+  nsCAutoString result;
> >+  nsresult rv = GetStringValue("uri", EmptyCString(), result);
> You don't need the temporary, just use aURI directly. [Why is this code
> duplicated for each type of addressbook?]

This code is duplicated because of the line "result.Insert(kMDBDirectoryRoot, 0);" and subsequent format - the different types of address book have different default uri formats.

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #25)
> (From update of attachment 221470 [details] [diff] [review] [edit])
> >-  rv = GetFileName(getter_Copies(prefStringValue));
> >+  rv = GetFileName(prefStringValue);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   
> >-  rv = properties->SetFileName(prefStringValue);
> >+  rv = properties->SetFileName(prefStringValue.get());
> >   NS_ENSURE_SUCCESS(rv, rv);
> I assume none of these are expecting null strings, right?

Some of these may be null strings. I don't believe its a problem that we need to handle here at this stage.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 221470 [details] [diff] [review]
Convert pref functions to use ACString for values v3 (checked in)

Checked in with nits addressed.

Comment 29

[Robert Accettura [:raccettura]]

Hmm... currently I'm using:
var selectedABDirectory = abooks.QueryInterface(Components.interfaces.nsIAbDirectory);
if(selectedABDirectory.directoryProperties.dirType == 2){ 
// do something
}

Should I no longer be doing this?  What should I be doing?  I'm trying to be compatible with Tbird 1.5+

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #29)
> Hmm... currently I'm using:
> var selectedABDirectory =
> abooks.QueryInterface(Components.interfaces.nsIAbDirectory);
> if(selectedABDirectory.directoryProperties.dirType == 2){ 
> // do something
> }
> 
> Should I no longer be doing this?  What should I be doing?  I'm trying to be
> compatible with Tbird 1.5+

This is most likely going to disappear, though I can't say when. The better option at the moment would be to do:

if (abooks instanceof Components.interfaces.nsIAbMDBDirectory)

Though looking at it we couldn't currently do this for outlook directories, so we may need to rethink this, or expose the outlook directory interface.

Comment 31

[Robert Accettura [:raccettura]]

Reason I ask is I'm using it in mozpod to detect if it's an AB we can sync to the iPod or not.

http://www.mozdev.org/source/browse/mozpod/src/mozpod/chrome/content/mozpod/syncmozpod.js

So I really need a way to get dirType to make sure we don't sync ldap (which won't work).

I'd love to figure out a way to get Outlook going, though doesn't seem likely.

Comment 32

[Mark Banner (:standard8)]

Attachment: Remove authDn from nsIAbDirectoryProperties (checked in)

Removes authDn from nsIAbDirectoryProperties/DIR_Server. We're able to set the dn directly on the nsIAbLDAPDirectory now. Note that it doesn't necessarily take effect straight away (bug 124553) but the change should be seen in prefs straight away if you re-open the ldap properties dialog.

Comment 33

[David :Bienvenu]

Comment on attachment 264249 [details] [diff] [review]
Remove authDn from nsIAbDirectoryProperties (checked in)

thx, Mark!

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 264249 [details] [diff] [review]
Remove authDn from nsIAbDirectoryProperties (checked in)

I checked this in last night.

Comment 35

[Mark Banner (:standard8)]

Attachment: Remove nsIAbDirectory::directoryProperties (checked in)

Removes the directoryProperties attribute from nsIAbDirectory. There's now sufficient other capability in nsIAb*Directory that we don't need the attribute (which is currently read-only anyway).

Comment 36

[David :Bienvenu]

Comment on attachment 269878 [details] [diff] [review]
Remove nsIAbDirectory::directoryProperties (checked in)

nice, thx, Mark.

Comment 37

[Mark Banner (:standard8)]

Attachment: WIP patch to remove nsIAbDirectoryProperties completely.

This patch is a work in progress version to remove nsIAbDirectoryProperties completely.

What doesn't work:
- Importing of address books (I've gotta rewrite that bit).
- For some reason I'm getting a personal address book underneath my first ldap directory with the same description.
- Outlook & OSX address books haven't been compiled tested.

I've only done limited address book testing on this so far, but it hopefully isn't far off the rest of it.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #37)
> Created an attachment (id=270206) [details]
> WIP patch to remove nsIAbDirectoryProperties completely.
> 
> This patch is a work in progress version to remove nsIAbDirectoryProperties
> completely.

I forgot to mention, this patch will make OSX directories need dirType set to 4 in prefs (not 3 as previously thought).

Comment 39

[Mark Banner (:standard8)]

Attachment: WIP patch to remove nsIAbDirectoryProperties completely v2

A slightly different approach this time. Before I was trying to remove DIR_AddNewAddressBook at the same time as nsIAbDirectoryProperties. The problem with that was trying to do two things at once which weren't ready for it.

So this solution extracts the necessary parts of nsIAbDirectoryProperties into function/interface parameters and keeps DIR_AddNewAddressBook for now.

This generally seems to be working, except I've broken the creation of new ldap address books. Given a day or two, I should hopefully be able to find out where I messed up ;-)

Comment 40

[Mark Banner (:standard8)]

Attachment: Remove nsIAbDirectoryProperties completely v3

This one is ready now - I've fixed the ldap create problem (I was comparing the full string as opposed to a sub string).

Comment 41

[David :Bienvenu]

Comment on attachment 274783 [details] [diff] [review]
Remove nsIAbDirectoryProperties completely v3

cool, do you need me to try this on Windows?

Comment 42

[Mark Banner (:standard8)]

(In reply to comment #41)
> (From update of attachment 274783 [details] [diff] [review])
> cool, do you need me to try this on Windows?
> 

The changes to windows are minor and I'm happy they will work as long as they compile (which I think they will).

Comment 43

[Mark Banner (:standard8)]

Bug 387404 no longer blocks this as I've found a way around it.

Comment 44

[Mark Banner (:standard8)]

Final patch checked in with a couple of minor bustage fixes.

This bug is now fixed :-)