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.

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:Joe.User@test.com N:User;Joe END:VCARD It's part of mail that I received from a Groupwise user.

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

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?

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

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

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.

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

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.

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

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 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.)

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

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

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

(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.)

(In reply to neil@parkwaycc.co.uk 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 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?

(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.

* 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 on attachment 8641738 [details] [diff] [review] Updated patch Is this still wanted? Unfortunately didn't get reviewed?