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

[Adenilson Cavalcanti]

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

Ship it!


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


/trunk/extragear/pim/googledata/contacts/googledataresource.cpp
<http://reviewboard.kde.org/r/2502/#comment2859>

    Concerning coding style, I'm following this one:
    http://www.ademar.org/texts/coding.html



/trunk/extragear/pim/googledata/contacts/googledataresource.cpp
<http://reviewboard.kde.org/r/2502/#comment2860>

    Trailing and leading spaces are evil (you can configure your code editor to display them).



/trunk/extragear/pim/googledata/contacts/googledataresource.cpp
<http://reviewboard.kde.org/r/2502/#comment2861>

    Idem.


- Adenilson


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/