[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:       "Stefano Avallone" <stavallo () unina ! it>
Date:       2010-01-05 23:29:47
Message-ID: 20100105232947.12281.25826 () 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

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


-----------------------------------------------------------
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]