[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-05 15:13:08
Message-ID: 20100105151308.552.5819 () localhost
[Download RAW message or body]


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