[Kde-pim] [Kde-extra-gear] [PATCH] Add support for multiple phone numbers in googledata

[Stefano Avallone]

On Monday 04 January 2010 09:05:04 Kevin Krammer wrote:
> I've looked through the patch for the resource and it looks quite ok
> (obviously Adenilson has the last say on both patches).
> 
> I think the two helper functions should start with lower case characters
> because this is the most common style for methods and functions anywhere in
> KDE.
> 
> The transfer of pointer ownership when setting values is a bit strange, the
> values themselves are transferred, the container array is not.
> Since the API docs of the libgcal function does not require any parameter
>  to be dynamically allocated on the heap, I think it would be safer not to
>  assign string pointers inside but to strdup() them and consequently let
>  the caller keep ownership of the array and the strings.

Kevin, many thanks for your quick review. Find attached a second version of 
the patches, hopefully addressing your comments.
I have also done a review request at reviewboard.kde.org as you suggested.

Cheers,
Stefano

 
> Cheers,
> Kevin
> 
> P.S.: for patches against KDE's repository I'd recommend doing a review
> request at reviewboard.kde.org
> This simplifies the review process a lot as one can see the code
>  surrounding the changes
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: googledata.diff
Type: text/x-patch
Size: 14957 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20100105/6ebaff65/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libgcal.diff
Type: text/x-patch
Size: 21497 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20100105/6ebaff65/attachment-0001.bin>
-------------- next part --------------
_______________________________________________
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/