[Kde-pim] Review Request: Add support for multiple phone numbers and email addresses to akonadi googledata agent

[Adenilson Cavalcanti]

> On 2010-01-05 15:13:13, Adenilson Cavalcanti wrote:
> > Dear Friend
> > 
> > Sorry about the long delay to answer (I was spending some time with my family and enjoying the new year holiday). First, I would like to thank you for adding this new feature to both libgcal and akonadi resources.
> > 
> > I have being working on both alone ever since, is always a nice surprise to receive patches. Recently, I was contacted by two other developers (Alexis and Christian, I'm forwarding this to both) concerning this very same subject.
> > 
> > As a matter of fact, Alexis already started to work on this at same time (as also support for birthday field), I guess it would be ideal to coordinate this efforts to avoid duplication of work.
> > 
> > Concerning the libgcal patch, I have some comments:
> > - I'm not very keen of acronyms (gcal_contact_get_emails_nr);
> > - All functions in libgcal are unit tested (as a matter of fact, it has around 80% of *real* code coverage). This means that any patch on it should come together with a unit test;
> > - Mike has cloned the libgcal repository to gitorious, my suggestion is for you guys to create a branch based on it and commit your work (http://gitorious.org/+libgcal-developers). The main advantage is that we make public and unified all the patches, while keeping safe the development history;
> > - All types in libgcal are 'abstract' types, in the sense that the user cannot access directly the fields (e.g. gcal_contact_t list is stored inside of gcal_contact_array). I think the same approach must be followed for contact's list of emails (and later addresses). So, returning and accepting char ** is not safe for libgcal users.
> > - The resources (i.e. strings) should be strdup'ed, making easier for memory resource management (as Kevin Krammer already noticed).
> > 
> > Best regards
> > 
> > 
> > Adenilson
> 
> Stefano Avallone wrote:
>     Thanks for your comments. I have cloned the libgcal repository on gitorious in order to commit the next version of my patch. I perfectly agree about the need of abstract types and I would propose the following functions:
>     
>     int gcal_contact_add_email_address(gcal_contact_t contact, char *field, char *type); // adds a single email address
>     int gcal_contact_delete_email_addresses(gcal_contact_t contact);  // delete all addresses, to be called before starting to add addresses
>     
>     char *gcal_contact_get_email_address(gcal_contact_t contact, int i);   // returns the i-th email address
>     char *gcal_contact_get_email_address_type(gcal_contact_t contact, int i);   // returns the type of the i-th email address
>     int gcal_contact_get_number_of_email_addresses(gcal_contact_t contact);   // returns the number of email addresses
>     
>     In such a way, the structure of email addresses is hidden to libgcal users. BTW, shall I modify the current structure (	char **emails_field; char **emails_type; int emails_nr;) or is it fine? Do you suggest different acronyms?
>     
>     cheers,
>     Stefano

Stefano

Nice to hear news from you. Only some remarks:

- maybe mail type could be exposed to users as a enumeration type instead of a string? So, gcal_contact_add_email_address(contact, "foo at whatever.com", HOME), where typedef enum { HOME, OFFICE, etc} GCAL_MAIL_TYPE is the last parameter (with default set to a sane value). Later it is internally mapped back to the proper string used by google server.

- concerning the number of email adresses, what about int gcal_contact_emails_count(gcal_contact_t) (a little less verbose) and where -1 means no email address set.

The current structure seems fine, just remember to respect memory ordering to not create holes inside of structure (you can use 'pahole' tool for inspect it).
http://lwn.net/Articles/206805/


Best regards


Adenilson


- Adenilson


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2502/#review3585
-----------------------------------------------------------


On 2010-01-04 23:58:39, Stefano Avallone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2502/
> -----------------------------------------------------------
> 
> (Updated 2010-01-04 23:58:39)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Currently, the akonadi googledata agent (and libgcal) does not support multiple phone numbers and email addresses per contact. This patch (along with a patch against libgcal, posted to the kde-extra-gear and kde-pim mailing lists) adds such support. An attempt is made to match Google labels for phone number types (work, home, etc.) and the label used by KAddressBook. Also, the group membership info associated with each Google contact is stored as a custom propoerty in Akonadi, thus this information is not lost when the contact is updated within KAddressBook.
> 
> 
> Diffs
> -----
> 
>   /trunk/extragear/pim/googledata/contacts/googledataresource.cpp 1066667 
> 
> Diff: http://reviewboard.kde.org/r/2502/diff
> 
> 
> Testing
> -------
> 
> I have done some tests with my Google account and it works. Also, a Chakra user reported it works for him, too.
> 
> 
> Thanks,
> 
> Stefano
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/