[Kde-pim] Request for merging akonadi-social-utils into kdepimlibs

[Allen Winter]

On Tuesday 25 September 2012 11:07:37 AM Martin Klapetek wrote:
> On Sat, Sep 22, 2012 at 1:28 AM, Allen Winter <winter at kde.org> wrote:
> 
> >
> > builds and installs ok <check>
> > no compile warnings <check>
> > test passes <check>
> >
> > missing Messages.sh (but ok since there are no i18n strings)
> > missing Mainpage.dox
> >
> 
> Will add one.
> 
Thanks.

> >  cmake/FindQJSON.cmake
> > (wait! cmake has a FindQJSON, can't you use that one?  There's a
> > FindQJSON.cmake in
> >  kdeplasma-addons, kde-workspace, kdevplatform, amarok... I wonder why?)
> >
> 
> Hm, I have none installed on my machine, so I guess neither package
> actually installs this file and so everyone just copies it over (I did as
> well from kdeplasma-addons). Also requiring a file from another package
> would mean a dependency, no? Should I just add it to kdepimlibs/cmake/ ?
> 
Yep, add to kdepimlibs/cmake.
Also move FindQtOAuth.cmake to kdepimlibs/cmake

> > there are a couple of very minor krazy issues (spelling, file doesn't end
> > with newline) but no big deal
> >
> 
> All fixed except missing license in that FindQJSON, because I don't know
> who the original author is and who should have the copyright. In the worst
> case, I can add a license saying something like "(c) KDE developers".
> 
> 
We can exclude FindQJSON.cmake from Krazy checking too.
Since it is "donated".

> > the style on a lot of the code is not compliant with our standard, but
> > again no big deal.
> > I'll want to fix that at some point though.
> > See http://community.kde.org/KDE_PIM/Development/CodingStyle
> 
> 
> Should be ok now.
> 
> 
> > Please remove the REQUIRED argument for QJSON in the top-level
> > CMakeLists.txt
> > as the following line with macro_log_feature() will handle bailing out for
> > you as needed.
> >
> 
> Done.
> 
> 
> >
> > You don't need the find_package(QJSON REQUIRED) in the
> > serializer/CMakeLists.txt do you?
> > Since already checked at the top-level
> >
> 
> True, that was a leftover.
> 
> 
> >
> > In general: this library looks fine and I have no objections to adding it
> > to kdepimlibs.
> > Would be a very nice addition.
> >
> 
> Thanks :)
> 
> I have one more question - I needed one small data-container class so I
> added it in socialfeeditem.h (so that file now has two class definitions).
> Is that ok or totally wrong? I didn't want to create&install another file
> for just 9 lines.
> 
That's ok:  we don't prohibit multiple classes defined in one header.

A couple other things I noticed:
 - we'll want to put CamelCase headers for this libary into kdepimlibs/includes
 - the installed library needs to have so-versioning.  do that by adding the line:
      set_target_properties(akonadisocialutils PROPERTIES VERSION ${GENERIC_LIB_VERSION} SOVERSION ${GENERIC_LIB_SOVERSION} )
   after the target_link_libraries(akonadisocialutils ...)
-  should this code become a subdir under kdepimlibs/akonadi?  installed as libakonadi-social.so ?
   with headers installed into akonadi/social ??

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