142940 - add support to LDIF export / import for generic AB columns

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

add support to LDIF export / import for generic AB columns 

needed to export / import the _AimScreenName column (or other generic columns)

the desired sample LDIF:

mozilla-ab-aimScreenName: myScreenName

as all good things, this should be extensible (no limit to generic columns) and 
pref based (hoo-ah).

Here come's the patch, baby!

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 2

[(not reading, please use [e-mail] instead)]

that patch includes the fix for #119360.

they could land together.  I'll attach a diff of diffs, so that reviewers can 
see what's new.

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: what's not part of the fix for #119360

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: fixed a string issue

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: what's not part of the fix for #119360

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: mozilla part of the fix (119360 has landed)

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: fix comment, fix Street -> State issue (#119360), fix nsAFlatCString -> nsACString issue.

Comment 8

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 83086 [details] [diff] [review]
fix comment, fix Street -> State issue (#119360), fix nsAFlatCString -> nsACString issue.

nsIAddrDatabase.idl
-------------------
a) can you include @param info in the doxygen comments for the new method?

b) how about using ACString or AUTF8String as appropriate for the last two
   params?

nsAddressBook.cpp
-----------------
c) if you make OPTIONAL_LDIF_MAP an NS_NAMED_LITERAL_STRING, the
length will be calculated at compile-time rather than run-time.  Also,
should it perhaps be named OPTIONAL_LDIF_EXPORT_MAP to make it clear
that it's only used for export?

d) genericValue.Length() is perhaps better expressed as 
   !genericValue.IsEmpty().

e) counting down to 0 inside loops may be a bit faster, since CPUs
   tend to have "compare with 0" instructions.

nsTextAddress.cpp
-----------------
f) Why put the generic attributes in the "m" case?  I don't think they
   should be required to be with "mozilla".

Comment 9

[(not reading, please use [e-mail] instead)]

bringing over status and keywords from the bug this depends on.

I hope to address dmose's comments soon.

Comment 10

[(not reading, please use [e-mail] instead)]

here comes a new patch, one that addresses all of dmose's comments except for 
two:

1)

> e) counting down to 0 inside loops may be a bit faster, since CPUs
>    tend to have "compare with 0" instructions.

you've raised this before.  I'm not convinced it's worth it.

The reason I like the way I've done it, is I think it's more readable than 
loops that count down, especially in this case when I'm going from n-1 to 0.

2)

>f) Why put the generic attributes in the "m" case?  I don't think they
>   should be required to be with "mozilla".

we decided that the non standard LDIF attributes would be of the 
form "mozillaFooBar".

we could allow attributes of any name, but then we need execute the CopyCharPref
() on every attribute.

the logical next step to your idea has an advantage:

we could do this for every ldif attribute, and define our attribute map
in prefs, and then remove Add*() methods on nsIAddrDatabase, since
AddRowValue() is the generic version of the Add*() methods.
The only difference is the generic version doesn't (currently) cache the mork 
token.

it would simplify nsTextAddress.cpp, and if the import map is defined in prefs, 
it would allow us to extend mozilla to handle other future LDIF.

meaning, if some popular application exports notes as "theseAreMyNotes"
we could add support for that application's LDIF by doing this:

pref("mail.addr_book.import_ldif_map.thesearemynotes", "Notes");

I think this is getting beyond the scope of this bug.  how about I take this to 
another bug, and leave it is as is (where we only support mozillaFooBar 
attributes) for now.

while you decide, I'll attach my new patch for a second review.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch, v2. address all but two of dmose's comments.

Comment 12

[(not reading, please use [e-mail] instead)]

another reason to wait on what I suggested:

this is slated to go into the 1.0 branch.  this current fix is lower risk than 
than what I propose to do next.  What I propose to do next would be trunk only, 
where bigger, riskier change (with not a lot of reward) should go.

Comment 13

[Roland Felnhofer]

I don't know if that really belongs to this bug - maybe I should have mentioned
that already when you worked on bug 119360. Unfortunately I haven't had enough
time at that time. How ever here are some commends on the LDIF (Inport /) Export
format:

1) The attribute 'c' ('countryName') is used but the objectclass 'country' isn't.
So we use an attribute without it's objectclass.

2) In addition the following 3 attributes are used which are proprietary Mozilla
attributes without the 'mozilla' prefix. It is still possible that they are
defined by RFCs but I haven't found any reference. So I you feel I'm wrong
please  evaluate what I've said. 
To keep all Mozilla internal attributes consistent I would recommend the following:

workurl -> mozillaWorkUrl
homeurl -> mozillaHomeUrl
custom1 -> mozillaCustom1
custom2 -> mozillaCustom2
custom3 -> mozillaCustom3
custom4 -> mozillaCustom4

3) I'm well aware that we still use 'xmozillanickname' and
'xmozillausehtmlmail'. For consistency it would be nice if we could change them
to 'mozillaNickname' and 'mozillaUseHtmlMail'. But there change might have a
broader impact that point 1 and 2

I think that point 1) can be easily done and it's risk is extremely small. It's
only used in the LDIF import and export routine and nowhere else.

Item 2 might have a little broader impact on existing code, depending if we also
want to update the LDAP part to support that newly named attributes there. To do
so I would highly appreciate but even to see a more homogeneous format in the
LDIF files I would see as a step forward in better ( and more consistent) LDAP
support within Mozilla

Item 3 might even have a broader impact for people already using Mozilla's LDAP
integration. 

So what are you thinking about that?

Comment 14

[(not reading, please use [e-mail] instead)]

> I don't know if that really belongs to this bug

your comments don't belong in this bug.  can you take them to another bug?

Comment 15

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 84121 [details] [diff] [review]
patch, v2.  address all but two of dmose's comments.

1) nsIPref is deprecated; new code should use nsIPrefBranch.  If you could fix
the new usages of that introduced by this patch, that would be great.

2) Can you add a comment explaining why the generic attribute test lives under
the "m" clause of parsing case so that people reading the code will know?

Making the attributes all generic would be a fine thing; though perhaps even
better would be to do it by reading an LDAP schema file rather than the prefs.

Also, I don't know if I agree that the direction of counting is more readable
as written, but the perf gain is tiny if any, and import/export isn't a perf
critical path, so I won't push it here.

Comment 16

[(not reading, please use [e-mail] instead)]

> 1) nsIPref is deprecated; new code should use nsIPrefBranch.  If you could fix
> the new usages of that introduced by this patch, that would be great.

good catch, I'll take care of it.

> 2) Can you add a comment explaining why the generic attribute test lives under
> the "m" clause of parsing case so that people reading the code will know?

will do.  I'll add a comment to the import code explaining why the code is in 
the "m" clause.

> Making the attributes all generic would be a fine thing; though perhaps even
> better would be to do it by reading an LDAP schema file rather than the prefs.

we can take this issue to the spin off bug I'll create once this bug is done.

> Also, I don't know if I agree that the direction of counting is more readable
> as written, but the perf gain is tiny if any, and import/export isn't a perf
> critical path, so I won't push it here.

thanks for being reasonable.

Here comes a new patch...

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 18

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 84359 [details] [diff] [review]
updated patch

r=dmose

Comment 19

[David :Bienvenu]

Comment on attachment 84359 [details] [diff] [review]
updated patch

sr=bienvenu

Comment 20

[(not reading, please use [e-mail] instead)]

fixed on trunk only.

Comment 21

[(not reading, please use [e-mail] instead)]

as promised, here's a spun off bug about the potential code cleanup:

http://bugzilla.mozilla.org/show_bug.cgi?id=145891

Comment 22

[Roland Felnhofer]

I filed Comment #13 as a new bug 146142

Comment 23

[Ninoschka Baca]

Trunk build 2002-05-28: WinMe, Linux RH 7.1, Mac 9.1
Verified Fixed.

Comment 24

[scottputterman]

adding adt1.0.1-.  Let's get this in the next release.