[Kde-pim] Review Request: Add new "default" fields to KPIMIdentities::Identity

[Robert Mathias Marmorstein]

> On March 14, 2012, 9:47 p.m., Ingo Klöcker wrote:
> > kalarmcal/CMakeLists.txt, lines 12-13
> > <http://git.reviewboard.kde.org/r/104181/diff/2/?file=53106#file53106line12>
> >
> >     Why is this needed? Because it's needed indirectly via indentity.h? That feels like the wrong solution. Shouldn't that fact that kpimidentities is needed automatically add the needed kabc includes? (I have to admit that I have no idea how resolution of indirect dependencies works in CMake.)

Yes, since identity.h pulls in addressee.h and kalarmcal pulls in identity.h, I had to add this to get everything to compile.  I dislike the additional dependency, but addressee.h is actually generated by a script and resides in the build folder, so it can't be automatically pulled in.  I think this gives weight to your argument above that this is perhaps the wrong solution to the problem.


> On March 14, 2012, 9:47 p.m., Ingo Klöcker wrote:
> > kpimidentities/identity.h, line 71
> > <http://git.reviewboard.kde.org/r/104181/diff/2/?file=53108#file53108line71>
> >
> >     This does not seem to be used anywhere.

Oops.  That's a hold-over from a previous attempt at this.  If I change the code to use setProperty I will need it, though, and it will no longer be unused.


- Robert Mathias


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


On March 14, 2012, 4:54 p.m., Robert Mathias Marmorstein wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104181/
> -----------------------------------------------------------
> 
> (Updated March 14, 2012, 4:54 p.m.)
> 
> 
> Review request for KDEPIM and KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> The KABC::StdAddressBook class has been deprecated and KPIMIdentities is the suggested replacement.  Unfortunately, many of the fields that were available in StdAddressBook are missing in the Identity class.  It's possible to use the property() and setProperty() methods to store custom fields, but that means that different apps might use different field names for things like home phone number and locality (city).  
> 
> We use this functionality in KOffice to store information about the document author.  Internally, we can just use the property methods, but it would be nice to have interoperability with other applications (such as KAddressBook).  
> 
> This patch adds methods for accessing some of the missing fields (the ones we use in KOffice).  Some of the functionality of StdAddressBook is still missing -- instead of a list of phone numbers, I hardwired in a "Home" and "Work" phone number -- but it is a step forward.
> 
> This is code I am not that familiar with, but it seems pretty straightforward.  I welcome any comments/constructive criticism.
> 
> 
> Diffs
> -----
> 
>   kalarmcal/CMakeLists.txt 0c0e956 
>   kpimidentities/CMakeLists.txt 7b1569d 
>   kpimidentities/identity.h b0d0e7c 
>   kpimidentities/identity.cpp e1023b3 
> 
> Diff: http://git.reviewboard.kde.org/r/104181/diff/
> 
> 
> Testing
> -------
> 
> kdepimlibs compiles and passes the same unit tests it did before the change.  Unfortunately, I just switched my main development computer to Kubuntu from Archlinux (which made testing library changes very easy) and am still trying to figure out the best way to test library changes without clobbering the system packages.  Any suggestions/help would be much appreciated.
> 
> 
> Thanks,
> 
> Robert Mathias Marmorstein
> 
>

_______________________________________________
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/