[Kde-pim] Re: Review Request: Contact Feeder port to Nepomuk DMS

[Martin Klapetek]

> On July 31, 2011, 8:31 a.m., Volker Krause wrote:
> > agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp, line 137
> > <http://git.reviewboard.kde.org/r/102084/diff/3/?file=30169#file30169line137>
> >
> >     We should use KStandardDirs here for determining the storage locations, so it follows standard environment variables etc. See e.g. KStandardDirs::localxdgdatadir().

Done.


> On July 31, 2011, 8:31 a.m., Volker Krause wrote:
> > agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp, line 144
> > <http://git.reviewboard.kde.org/r/102084/diff/3/?file=30169#file30169line144>
> >
> >     some of the variables here could be made const

Probably yes, I'll add it.


> On July 31, 2011, 8:31 a.m., Volker Krause wrote:
> > agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp, line 167
> > <http://git.reviewboard.kde.org/r/102084/diff/3/?file=30169#file30169line167>
> >
> >     You changed given and family name to use "addNameX" methods, but not the other name parts (middle name, etc). Is this correct?

I think that depends on the cardinality in the ontologies, because some properties have setFoo only for lists and some only for strings. And the initial port used different headers, therefore the mixup. I'll fix it so it uses setFoo methods everywhere, creating lists from strings for list-only setters.


> On July 31, 2011, 8:31 a.m., Volker Krause wrote:
> > agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp, line 381
> > <http://git.reviewboard.kde.org/r/102084/diff/3/?file=30169#file30169line381>
> >
> >     Again a mix of addFoo/setFoo methods, is that intentional?

Explained above.


> On July 31, 2011, 8:31 a.m., Volker Krause wrote:
> > agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp, line 235
> > <http://git.reviewboard.kde.org/r/102084/diff/3/?file=30169#file30169line235>
> >
> >     Hm, this is quite some copy&paste code here (not your fault, it was before already). Do you think we can refactor that to have less duplication? Not a blocker anyway, we can always do that later.

I'll see what I can do. For now I'll submit updated patch without this change and look into this later.


- Martin


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


On July 28, 2011, 11:57 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102084/
> -----------------------------------------------------------
> 
> (Updated July 28, 2011, 11:57 a.m.)
> 
> 
> Review request for KDEPIM, Nepomuk and Christian Mollekopf.
> 
> 
> Summary
> -------
> 
> This is the port of Akonadi's Nepomuk-Contact-Feeder to the new Nepomuk SimpleResource API. It follows Christian Mollekopf's NepomukFeederAgent port.
> 
> 
> Diffs
> -----
> 
>   agents/CMakeLists.txt f742e86 
>   agents/nepomuk_contact_feeder/nepomukcontactfeeder.h 1e86efb 
>   agents/nepomuk_contact_feeder/nepomukcontactfeeder.cpp bf32ed2 
>   agents/nepomukfeeder/nepomukfeederagentbase.cpp 7133c75 
> 
> Diff: http://git.reviewboard.kde.org/r/102084/diff
> 
> 
> Testing
> -------
> 
> Created/updated a contact in KAddressBook, looked it up in Nepomuk database, all data were present.
> 
> 
> Thanks,
> 
> Martin
> 
>

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