GNOME Bugzilla – Bug 656603
[PATCH] Add support for generating vCard 2.1 in libebook
Last modified: 2011-10-19 08:07:48 UTC
Created attachment 193894 [details] [review] Patch for gnome-2-32 branch Libebook method e_vcard_to_string_vcard_21 (addressbook/libebook/e-vcard.c) returns empty string and throws warning to log, when trying to generate vCard in version 2.1 -- generally it is not implemented. RATIONALE: I'm implementing Phonebook Access Profile (PBAP) in obexd that uses EDS as data backend. It is essential for PBAP implementation to be able to send vCards in version 2.1, not only it is a default version (when remote party did not request one), furthermore most carkits and headsets seems to accept only 2.1 (i've tried to force sending 3.0, but it's rejected). EDS PBAP in obexd is almost finished, as for now it need extensive testing, but generally is fully functional (without generating proper vCard 2.1). ATTACHMENTS: Patches introducing generating vcards in version 2.1, formated for master, gnome-2-32, gnome-3-0.
Created attachment 193895 [details] [review] Patch for branch gnome-3-0
Created attachment 193896 [details] [review] Patch for master
I don't like how you are checking for valid properties with a simple strstr, because I could add a property called "ELL" and it would match (being a substring of "CELL"). How about doing a on-demand population of a hash table containing valid keys?
Of course i may use some more sophisticated method, but I'm not sure whether increasing complexity and time needed for this operation is worth it -- because is not that this "faulty" property may be inserted by accident (or maybe I'm wrong?) data is pulled from store and thats only label AFAIK user can't change them, i'm checking it only because some poorly (generally it may as well omit not recognized ones) written parsers might choke on it. Not sure what you mean by "on-demand population of a hash table containing valid keys", but my first thought was to call g_strsplit (once to be honest as it may be stored somehow) on it and then iterate over the array. "on-demand population of a hash table containing valid keys" like g_hash_table_insert (dict, "CELL", NULL) g_hash_table_insert (dict, "WORK", NULL) ... and then check if key is in dict?
Created attachment 194278 [details] [review] Patch for master (updated version)
Created attachment 194279 [details] [review] Patch for gnome-2-32 branch (updated)
Created attachment 194280 [details] [review] Patch for gnome-3-0 branch (updated)
Patches have been updated, as Ross suggested I've used HashTable for valid arguments in vCard 2.1. Time impact seems to be minimal (even when downloading whole google contact phonebook through BT).
I'm not sure whether Ross will finish review of your effort, but looking into the patch for master branch (which is basically the only one where I would accept this new feature (I call this a feature, and all bug master branch are frozen for new features)), it looks fine, except of some minor things: a) 'e_vcard_escape_semicolns' seems like a typo in the function name. I suppose the already existing 'e_vcard_escape_string' is not suitable for this, because the 2.1 standard doesn't escape all those other chars? b) evolution is strictly using GLib types, where in e_vcard_qp_encode() these are not used (you've there 'char' and 'int'). Similar in generate_dict_validator(). c) I usually use "=%02X" instead of yours "=%2.2X", though that's just matter of preference (supposing yours is also adding leading zero to the two-byte hexa value of the code). d) in e_vcard_to_string() I would strstr for 'CRLF "VERSION:2.1" CRLF' to ensure it picks the right line. Similar for 3.0 version check.
(In reply to comment #9) > I'm not sure whether Ross will finish review of your effort, but looking into > the patch for master branch (which is basically the only one where I would > accept this new feature (I call this a feature, and all bug master branch are > frozen for new features)), it looks fine, except of some minor things: > > a) 'e_vcard_escape_semicolns' seems like a typo in the function name. I suppose > the already existing 'e_vcard_escape_string' is not suitable for this, because > the 2.1 standard doesn't escape all those other chars? Sure it's typo, and yes there are different sets of "bad" chars. > b) evolution is strictly using GLib types, where in e_vcard_qp_encode() these > are not used (you've there 'char' and 'int'). Similar in > generate_dict_validator(). OK > c) I usually use "=%02X" instead of yours "=%2.2X", though that's just matter > of preference (supposing yours is also adding leading zero to the two-byte hexa > value of the code). > > d) in e_vcard_to_string() I would strstr for 'CRLF "VERSION:2.1" CRLF' to > ensure it picks the right line. Similar for 3.0 version check. Sure, it would be safer that way. So i'll fix these issues, and hopefully post new patch on sunday/monday.
Thanks. Thinking of it, the vCard is treated as case insensitive, while your GHashTable is case sensitive. It might not work with vCards which has attributes/parameters in lower case.
Created attachment 199228 [details] [review] Patch adding support for vCard 2.1 Fixed issues pointed in previous comments.
Thanks for the update. I made minor coding style changes and replaced strcasestr() function with strstr_nocase() from templates plugin from evolution, to avoid a requirement on the _GNU_SOURCE. I also extended tests/libebook/vcard/dump-vcard.c to print the vCard as a 2.1 version, where I realized that you do not fold long attributes. It might not be an issue, right? I suppose not, then I'm committing this. Thanks for the work.
Created commit cca25e9 in eds master (3.3.1+)
Thanks! vCard 2.1 spec just mention that long lines _might_ be split at spaces, but it's not necessary - I'm not folding it. On the other hand folding is necessary for quoted-printable, so it's used only for actual data. Cheers!
Ah, good, I didn't know that. Thanks.