After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 427469 - Tasks: I imported a vcard file ...
Tasks: I imported a vcard file ...
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Contacts
unspecified
Other All
: Normal normal
: ---
Assigned To: Milan Crha
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2007-04-08 01:19 UTC by Jonathan Kamens
Modified: 2007-08-06 14:53 UTC
See Also:
GNOME target: ---
GNOME version: 2.17/2.18


Attachments
e-d-s patch (2.87 KB, patch)
2007-05-07 17:10 UTC, Milan Crha
needs-work Details | Review
evolution patch (1.32 KB, patch)
2007-05-07 17:33 UTC, Milan Crha
committed Details | Review
proposed e-d-s patch ][ (2.73 KB, patch)
2007-06-04 07:38 UTC, Milan Crha
committed Details | Review

Description Jonathan Kamens 2007-04-08 01:19:12 UTC
I imported a vcard file that looks like this into Evolution:

BEGIN:VCARD
N:Lastname;Firstname;;;
ADR;DOM;HOME:;;Street Address;City;State;ZIP;
TEL;HOME:+1-XXX-XXX-XXXX
EMAIL:XXX@XXX.XXX
END:VCARD

It's a completely valid vcard.  Evolution got it wrong in two different ways.  First of all, next to the email address in the preview display it says "((null))".  If the type of the email address wasn't specified, it should default to Other, probably.  Second, it didn't import the address at all.



Distribution: Fedora release 6.92 (Rawhide)
Gnome Release: 2.18.0 2007-03-23 (Red Hat, Inc)
BugBuddy Version: 2.18.0
Comment 1 André Klapper 2007-04-11 15:25:04 UTC
valid? why should that one be valid?!

ADR;DOM;HOME:;;

also, where is the vcard VERSION statement?
Comment 2 Jonathan Kamens 2007-04-12 02:32:18 UTC
I do not understand what your problem is with the ADR line, and the vCard specification does not require a VERSION statement.

Please be more specific about why you think this vCard is invalid.

I realize that MS is not exactly known for standards compliance, but it is worth noting that both Outlook and Windows Mobile accept vCards in this format correctly and without complaint.
Comment 3 André Klapper 2007-04-12 11:37:22 UTC
http://tools.ietf.org/html/rfc2426 , sections 3.6.9 ("The VERSION property MUST be present in the vCard object") and 3.2.1 (the TYPE property is missing for ADR and TEL).

closing as NOTABUG.
Comment 4 Jonathan Kamens 2007-04-12 13:17:03 UTC
Sorry, I was wrong about the VERSION property being optional, but the same specification you cited to prove that it is required also indicates that the TYPE property is optional and specifies what the default should be if it's missing, so if you're not defaulting TYPE properly for ADR and TEL, then you've still got a bug.
Comment 5 André Klapper 2007-04-12 14:04:51 UTC
yeah, TYPE is an optional, sure, but that's not what we're talking about here:

The TYPE parameter values can include "dom" to
   indicate a domestic delivery address; "intl" to indicate an
   international delivery address; "postal" to indicate a postal
   delivery address; "parcel" to indicate a parcel delivery address;
   "home" to indicate a delivery address for a residence; "work" to
   indicate delivery address for a place of work; and "pref" to indicate
   the preferred delivery address when more than one address is
   specified. These type parameter values can be specified as a
   parameter list (i.e., "TYPE=dom;TYPE=postal") or as a value list
   (i.e., "TYPE=dom,postal"). This type is based on semantics of the
   X.520 geographical and postal addressing attributes. The default is
   "TYPE=intl,postal,parcel,work". The default can be overridden to some
   other set of values by specifying one or more alternate values. For
   example, the default can be reset to "TYPE=dom,postal,work,home" to
   specify a domestic delivery address for postal delivery to a
   residence that is also used for work.

so what should "ADR;DOM;HOME:;;" be?!
Comment 6 Jonathan Kamens 2007-04-12 14:23:35 UTC
TYPE= is optional in VCARD 2.1.  Please see the example in section 2.3.1 of the vCard 2.1 specification.  See also section 2.1.2 of the same specification, which says, "The value string can be specified alone in those cases where the value is unambiguous."

Is it your intent to reject VCARD 2.1 inputs?
Comment 7 André Klapper 2007-04-12 15:41:24 UTC
TYPE= is also optional in 3.0, that's not the issue.
again: what should "ADR;DOM;HOME:;;" be?! can you answer that, please? :-)

nobody wants to reject any inputs if they are valid files, but evo doesn't have a crystalball implemented for any broken apps producing broken outputs...
Comment 8 André Klapper 2007-04-12 15:42:53 UTC
oops, that "ADR;DOM;HOME:;;" part was already answered, sorry... can i get a link to the 2.1 spec? thanks in advance.
Comment 9 Jonathan Kamens 2007-04-12 16:45:24 UTC
Please see http://www.imc.org/pdi/pdiproddev.html.  There are links there to the spec in a number of different formats.

"ADR;DOM;HOME:" is equivalent to "ADR;TYPE=DOM;TYPE=HOME:" or "ADR;TYPE=DOM,HOME:".

And incidentally, as far as I can tell from the RFC, "TYPE=" is *not* optional in 3.0.  It says in section 5, 'The `TYPE=' prefix to parameter values is required. In[VCARD] this was optional."
Comment 10 André Klapper 2007-04-12 19:19:46 UTC
the question is if there's anything in that 2.1 definition that says that version is optional...
Comment 11 Milan Crha 2007-05-04 09:19:03 UTC
(In reply to comment #10)
> the question is if there's anything in that 2.1 definition that says that
> version is optional...
> 

in spec 2.1 file from link by Jonathan is written "This property must appear within the vCard data stream."

There has been made some progress (I think as side effect of other bug), because with that email there is not shown "((null))" now, but "()" instead. In 2.1 the type in EMAIL is optional, like in TEL and ADR. So it maybe changed to "OTHER" like in ADR, I think.

When I change original vcard from comment #1 on this one:

BEGIN:VCARD
VERSION:2.1
N:Lastname;Firstname;;;
ADR;TYPE=DOM;TYPE=HOME:;;Street Address;City;State;ZIP;
TEL;HOME:+1-XXX-XXX-XXXX
EMAIL:XXX@XXX.XXX
END:VCARD

then it ignore ADR as well, so I think the problem isn't about missing VERSION property. I'm not sure why, but when I remove that TYPE=DOM from ADR then it is also shown in Evolution, otherwise it's imported but one could not see it in UI.
Comment 12 Milan Crha 2007-05-07 14:23:41 UTC
(I'm sorry, I mean comment #0 in the above comment.)

patches from bug #313221 solve part of this bug (ADR is shown with them, regardless of TYPE values ordering.)
Comment 13 Milan Crha 2007-05-07 17:10:20 UTC
Created attachment 87723 [details] [review]
e-d-s patch

for evolution-data-server;

You need to apply patch from comment #12 too for right behavior. When you use original vCard from statement, then while parsing input, TYPE params are added twice for ADR, except of only one param TYPE with two values. I changed e-vcard.c: e_vcard_attribute_add_param in a way it tests for duplicities and merge values if needed. I think it's better do here than in read_attribute_params.
Comment 14 Milan Crha 2007-05-07 17:33:53 UTC
Created attachment 87724 [details] [review]
evolution patch

for evolution.

with this patch the TYPE=OTHER is added for EMAILs without TYPE=HOME and TYPE=WORK param. With all that patches (two here and two at bug #313221) together the whole bug could be fixed.
Comment 15 Ross Burton 2007-06-03 11:37:29 UTC
Comment on attachment 87724 [details] [review]
evolution patch

Looks good to me.
Comment 16 Ross Burton 2007-06-03 11:40:40 UTC
Comment on attachment 87723 [details] [review]
e-d-s patch

This needs to be as fast as possible as it will be called *a lot*.

Instead of using the accessors, directly access the fields, i.e. param->name rather than e_vcard_attribute_param_get_name(), attr->params rather than e_vcard_attribute_get_params().

Also "if (foo) {", you often miss the space between ) and { or add a space before foo.

I'm not entirely convinced that EVCard should be handling this though, I'm leaning towards saying that applications should iterate over every TYPE parameter instead of only the first.
Comment 17 Milan Crha 2007-06-04 07:38:44 UTC
Created attachment 89313 [details] [review]
proposed e-d-s patch ][

for evolution-data-server;

I changed it as Ross suggested. With that handling, I hope it's better to handle it here than have same code on each place where you need to add parameter to attribute.
Comment 18 Srinivasa Ragavan 2007-06-09 20:27:15 UTC
Can we get this for 2.11.4?
Comment 19 Srinivasa Ragavan 2007-06-30 20:15:02 UTC
Ross: Can you review this for 2.11.5?
Comment 20 Ross Burton 2007-08-06 10:48:46 UTC
The EDS patch looks good to me.
Comment 21 Ross Burton 2007-08-06 14:17:15 UTC
Commit this too.
Comment 22 Milan Crha 2007-08-06 14:53:26 UTC
e-d-s part committed to trunk. Committed revision 7925.
evo part committed to trunk. Committed revision 33959.