221991 - Add full support for vCard 2.1

Status: RESOLVED WORKSFORME

(MailNews Core :: Address Book, defect)

Description

[Ian Neal]

After the landing of bug 14373 mozilla does support vCards to a limited extent
but really needs to have full support for vCard 2.1. This means being able to
deal with both home and work addresses.

Comment 1

[Boris Folgmann]

Is this the reason why the following "Joe User.vcf" is displayed as a text
attachment and not as a beautiful, graphical vcard in mozilla 1.7.3?

BEGIN:VCARD
VERSION:2.1
X-GWTYPE:USER
FN:Joe User
ORG:;Only a test
EMAIL;WORK;PREF;NGW:[e-mail]
N:User;Joe
END:VCARD

It's part of mail that I received from a Groupwise user.

Comment 2

[Ian Neal]

Probably not, I suspect that groupwise is sending the vcard out with the wrong
mime type. If you check by looking at message source it should have a mime type
of something like text/x-vcard

Comment 3

[Boris Folgmann]

You're right, Groupwise seems to be Crapware:

Content-Type: text/plain; name="Joe User.vcf"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="Joe User.vcf"

Could you add a fallback for *.vcf files to mozilla, so this kind of mails look
better?

Comment 4

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

re-assign to nobody, I'm not planning to work on this.

Comment 5

[Fabian Rodriguez:MagicFab]

The MoreColsForAddressBook extension supports forwarding contacts as vCard as well as exporting/importing in vCard format:
http://nic-nac-project.de/~kaosmos/morecols-en.html

Comment 10

[[e-mail]]

So, the vCard support works like this.
There is a library originally written in YACC, however the original source file is lost and only the generated nsVCard files are available. It parses vCard 1.2 format into a vObject, which is another library nsVCardObj which also knows how to serialise a vObject into a string.

A vObject is a set of properties, each of which is a vObject. vObjects can also have values. Simple properties, such as display name, are therefore represented as a vObject with a value. Contact names are represented as a name vObject which has given and family name subproperties with the appropriate values. Telephone numbers are slightly more complicated, as the vObject not only has the value of the telephone number itself, but it also has a subproperty that identifies which type of telephone number it is (work/home/etc.) Addresses are more complex still, as the address property has an optional property that identifies which type of address it is, plus properties for the components of the address.

To serialise a vCard the nsAbCard object creates a vObject and serialises it to a string. In this case things are relatively straightforward; you just have to make sure that you have tagged the right property vObjects with the right values.

To parse a vCard the address book manager calls the nsVCard's parse_MIME method which creates a vObject, it then uses nsVCardObj methods to enumerate the vObject's properties and converts them into an nsAbCard object. This is where things start to get tricky, as the current code assumes that each property name only exists once, except in the case of telephone numbers, where the type of telephone number is a subproperty of the telephone number itself. Unfortunately this doesn't work for home addresses.

Comment 11

[[e-mail]]

Attachment: Import more fields

This recognises the following additional vCard 2.1 fields for import:
url;home: (Private Website)
adr;home:;;;;;; (Private Address)

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8580803 [details] [diff] [review]
Import more fields

Review of attachment 8580803 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. Though is sounds like vcard support wants to be completely reimplemented.

Comment 13

[[e-mail]]

Attachment: Import even more fields and export them too

Note that to actually export the fields you'll need the patch in bug 1139965.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8584071 [details] [diff] [review]
Import even more fields and export them too

Review of attachment 8584071 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +1390,5 @@
> +  const char *objName = vObjectName(vObj);
> +  int low = 0, high = MOZ_ARRAY_LENGTH(handlers);
> +  while (high > low)
> +  {
> +    int mid = (low + high) >> 1;

I don't understand what this function is doing with the mid, low, high.
Could you clarify that, probably as code comments too.

Comment 15

[[e-mail]]

Comment on attachment 8584071 [details] [diff] [review]
Import even more fields and export them too

>+    t = nullptr;
>+
>     rv = GetPropertyAsAString(kCompanyProperty, str);
>     if (NS_SUCCEEDED(rv) && !str.IsEmpty())
>     {
>-        t = isAPropertyOf(vObj, VCOrgProp);
>         if (!t)
>             t = addProp(vObj, VCOrgProp);
Ben was asking about this change. The problem here is that there are now two properties, one for home, one for work, and the simplistic isAPropertyOf call has no way of distinguishing the two. However, the property we are interested in is the same for all fields in the property, so we don't actually need to look it up each time as long as we clear it in between properties.

>+  int low = 0, high = MOZ_ARRAY_LENGTH(handlers);
>+  while (high > low)
>+  {
>+    int mid = (low + high) >> 1;
This is a binary search; there's a generic routine in mfbt that finds a binary insertion point but that's not what I'm looking for. (Although now that I think of it I could perhaps provide a custom comparator that also carried out the action when it finds a match.)

Comment 16

[[e-mail]]

Attachment: First C++ lambda in MailNews!

BinarySearchIf works much better when using one of those new C++ lambdas.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8600170 [details] [diff] [review]
First C++ lambda in MailNews!

Review of attachment 8600170 [details] [diff] [review]:
-----------------------------------------------------------------

I think someone more c++ should review this... some tests would also be nice :)

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +530,5 @@
>      if (!str.IsEmpty()) {
>          myAddPropValue(vObj, VCFullNameProp, str.get(), &vCardHasData);
>      }
>  
> +    t = nullptr;

why are you setting this for some, but not all?

@@ +538,1 @@
>          if (!t)

and these are now if(true) basically

Comment 18

[Ronald Killmer]

The VCard spec now at version 4.0 dated Aug, 2011 is at this URL.
https://tools.ietf.org/html/rfc63501

A quick look and there are other cases of home/work fields we should support

Comment 19

[[e-mail]]

(In reply to Magnus Melin from comment #17)
> > +    t = nullptr;
> why are you setting this for some, but not all?
I'm setting it when it's the parent of a group of properties. The old code used to use this format:
// repeat for each property with a parent
if (need to set property with parent) {
  try to find parent;
  if (not found)
    create parent;
  add property;
}
I've changed it to this format:
// repeat for each parent
forget last parent;
// repeat for each property with the same parent
if (need to set property with parent) {
  if (no parent yet)
    create parent;
  add property;
}

> >          if (!t)
> and these are now if(true) basically
Notice that the old code tried to find the parent even in the first block for a given parent which therefore couldn't possibly have a parent. My code does a test too but the compiler should find it easier to optimise away ;-) (The test is of course necessary in subsequent blocks for a given parent.)

Comment 20

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #10)
> So, the vCard support works like this.
> There is a library originally written in YACC, however the original source
> file is lost and only the generated nsVCard files are available.

You can actually reverse engineer the original yacc file, since the compilation was done in a debug mode that emits the rule information. I know I did that once, but I'm not sure if I've kept the resulting code anywhere.

Honestly, the vObject library seems sufficiently squirrely that it would probably be cleaner to just have the emission code build the string by hand rather than going through vObject. Especially since the code is basically a hard-coded list of properties, which are treated invariably as strings or (fixed) list-of-strings anyways, which would be extremely trivial to write helper functions for.

I've been using RFC 6350 to guide the structure of a vCard file, although that admittedly is a vCard 4.0 instead of 2.1 (!).

Comment 21

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8600170 [details] [diff] [review]
First C++ lambda in MailNews!

Review of attachment 8600170 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't pass tests, unfortunately.

Even then, I'd like there to be more tests (we only have one test for converting to a vcard, and another test that only checks that we don't crash on parsing a vcard). Actually, we have tests for vcard import in the importer, but they're woefully underpowered.

What version of vCard are you specifically trying to target? Doing some digging, there are slight incompatibilities between vCard 3 and 4 (the two versions for which documents exist in RFC form), and vCard 4 has a pretty scary errata list (aka "oh man, we botched major parts of the specification and opened the door to potential significant interoperability issues). Since the vCard is designed to be more or less backwards-compatible, it's not as important in the vCard emitter, but it's a much bigger deal on the parser. The fact that the vCard parser is effectively black-boxed due to its yacc history makes it even more difficult to ascertain how much of the newer stuff we should/can support (it's also impossible to change that parser without rewriting it from scratch, should we need to).

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +530,5 @@
>      if (!str.IsEmpty()) {
>          myAddPropValue(vObj, VCFullNameProp, str.get(), &vCardHasData);
>      }
>  
> +    t = nullptr;

As I mentioned in my earlier comment, this function would probably be much clearer if it were written to do the string building itself instead of building VObjects and then using the old library to emit the VObject to a string. Especially since the code is largely "\n".join(name + ":" + ";".join(card[prop] for prop in props) for (name, prop) in convList) [that's Pythonic, I know].

I'm not going to hold you to that for an r+, though.

Outside of that consideration, you should add comments emphasizing that the t is a temporary variable to build property groups. Actually, better yet, you could abstract a function to handle the property grouping, since at the moment, it's clear that you're duplicating a ton of boilerplate code in a very breakage-prone way.

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +36,5 @@
>  #include "nsComponentManagerUtils.h"
>  #include "nsIIOService.h"
>  #include "nsAbQueryStringToExpression.h"
>  #include "mozilla/ArrayUtils.h"
> +#include "mozilla/BinarySearch.h"

Kudos on you for finding and using the binary search function! Especially since I know the m-c people were concerned about potential security vulnerabilities due to incorrect hand-rolled binary searches.

@@ +1304,5 @@
> +  uri->SchemeIs("file", &file);
> +  if (file)
> +    aCard->SetPropertyAsAUTF8String(kPhotoTypeProperty, NS_LITERAL_CSTRING("file"));
> +  else
> +    aCard->SetPropertyAsAUTF8String(kPhotoTypeProperty, NS_LITERAL_CSTRING("web"));

<aside>What should we be doing for data: URLs? I'd imagine they'd have to be up there in terms of use...</aside>

@@ +1317,5 @@
> +  {
> +    VObject *prop = nextVObject(&i);
> +    nsAdoptingCString cardPropValue(getCString(prop));
> +    if (!cardPropValue)
> +      continue;

I don't know what the VObject library is producing in terms of structure, but the fact that some of your parsing functions have the unknown value continue while others return is a major incongruity. Without a test suite that touches any of these values, it's hard to ascertain if one or both forms are right.

This kind of similar-but-almost-the-same structure decoding seems to me like it deserves some sort of unified method to handle it. Unfortunately, C++ doesn't let you template on strings… ☹

@@ +1359,5 @@
> +    if (cardPropValue[i] == '-')
> +      bits |= 16 << i;
> +    else if (cardPropValue[i] < '0' || cardPropValue[i] > '9')
> +      return;
> +  }

This is clever code, and clever code really needs to have comments explaining its cleverness. It's not immediately obvious that the bits value is designed to have the length be encoded in one nibble and the positions of various dashes be encoded in the bits of higher-order nibbles.

Also, it would be helpful to cite the source for the various formats (I'm assuming you're using the date ABNF production of RFC 6350 here).

@@ +1457,5 @@
> +  const char *objName;
> +  const char *cardProperty;
> +};
> +
> +static Handler handlers[] = {

Since you do a binary search over this array, it is absolutely imperative that this array be in sorted order. This needs to be mentioned in a comment (or perhaps better yet, some sort of runtime (or static? not sure if that's possible without really crazy templating...) assertion). The fact that names are hidden behind macros make it less obvious that they're in sorted order.

@@ +1476,5 @@
> +static void convertPropValue(VObject *vObj, nsIAbCard *aCard)
> +{
> +  size_t pos;
> +  const char *objName = vObjectName(vObj);
> +  if (BinarySearchIf(handlers, 0, MOZ_ARRAY_LENGTH(handlers),

IIRC, mozilla::ArrayLength(handlers) would be the preferred name here, would it not?

Comment 22

[[e-mail]]

(In reply to Joshua Cranmer from comment #21)
> This doesn't pass tests, unfortunately.
We have tests?

> What version of vCard are you specifically trying to target?
2.1, I think.

> Especially since the code is largely "\n".join(name + ":" + ";".join(card[prop]
> for prop in props) for (name, prop) in convList) [that's Pythonic, I know].
Assuming I counted correctly there are about 30 properties of which only 7 are that simple.

> <aside>What should we be doing for data: URLs? I'd imagine they'd have to be
> up there in terms of use...</aside>
IIRC our UI treats them as web links rather than local files.

> 
> @@ +1317,5 @@
> > +  {
> > +    VObject *prop = nextVObject(&i);
> > +    nsAdoptingCString cardPropValue(getCString(prop));
> > +    if (!cardPropValue)
> > +      continue;
> 
> I don't know what the VObject library is producing in terms of structure,
> but the fact that some of your parsing functions have the unknown value
> continue while others return is a major incongruity.
Basically the idea is that I traverse the VObject looking for values that I can set card properties for. If I'm in an inner loop then I need to continue looking for properties. If I'm not in an inner loop then I need to return to the outer loop.

> This kind of similar-but-almost-the-same structure decoding seems to me like
> it deserves some sort of unified method to handle it. Unfortunately, C++
> doesn't let you template on strings… ☹
I know, otherwise I would have templated the four simple properties.

> Also, it would be helpful to cite the source for the various formats (I'm
> assuming you're using the date ABNF production of RFC 6350 here).
Hmm, it looks like I forgot --MM for some reason.

> IIRC, mozilla::ArrayLength(handlers) would be the preferred name here, would
> it not?
I don't like using it because it's not a compile-time constant in MSVC builds. (It is an optimise-time constant in certain places, of which this might be one.) MOZ_ARRAY_LENGTH is type-safe in C++ anyway.

Comment 23

[[e-mail]]

Attachment: Updated patch

* Updated for bitrot.
* Added a bunch of comments to clarify the translate to vcard code. I tried to use functions to simplify the property grouping but it actually seemed to become more complicated. I considered doing something using macros but that's not necessarily better.
* Added a bunch of comments to the import code too.
* Tweaked the control flow in the import code.
* Added support for the --MM date format.
* Added a debug assertion for the handler array.
* Updated the only test I could find that was going to break.

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 8641738 [details] [diff] [review]
Updated patch

Is this still wanted?

Unfortunately didn't get reviewed?

Comment 25

[Takanori MATSUURA]

It looks attachment 8641738 [details] [diff] [review] did not review because the review flagged without the reviewer set.

Comment 26

[Geoff Lankow (:darktrojan)]

I'm calling this WFM, although strictly speaking we only have full support for vCard 3 and 4, and read-only support for 2.1. That should be suitable for almost everything these days.