'Re: [Kde-pim] Review Request: Add support for multiple phone'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Add support for multiple phone
From:       "Adenilson Cavalcanti" <cavalcantii () gmail ! com>
Date:       2010-01-06 18:44:37
Message-ID: 20100106184437.1583.34261 () localhost
[Download RAW message or body]

> 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@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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 