[Kde-maemo] kdelibs modularizing (kde-mobile)

[Kevin Ottens]

On Tuesday 27 April 2010 22:02:33 Alexander Neundorf wrote:
> kdelibs_cmake_profiles.patch:
> Looks good, but I would put the information not in a separate
> KDEPlatformProfile.cmake file, but just in the already existing
> KDELibsDependencies.cmake file.
> 
> (if you have a look at that file you'll notice that the filename is just
> a "historic" relict and that it doesn't contain any dependency information
> anymore, this is now in the exported target-files).

Yeah, thought about it already, and after all started a separate files for the 
following reasons:
1) The indeed unclear name of KDELibsDependencies.cmake and I didn't want to 
worsen the situation;
2) To keep the profile definition readable an maintainable, putting the logic 
in CreateKDELibsDependencies.cmake it'd be buried with a lot of other details 
(that said CreateKDEPlatformProfile.cmake could write into 
KDELibsDependencies.cmake);
3) The platform profile definition has to be processed as early as possible 
(in particular before the add_subdirectory directives) while the other 
definitions can come up later apparently;
4) And, it's really meant to be a controlling file also for other modules 
forming our platform like kdepimlibs but which aren't kdelibs, a 
KDELibsSomething name would just give the wrong signal IMO.
 
> ...we might actually rename the KDELibsDependencies.cmake to
> KDELibs4Settings.cmake or something like that, and we might also think
> about installing these files somewhere in libs/ instead of share/...
> We'll have to make sure there are no compatibility issues if we rename/move
> this stuff.

Right, would make a lot of sense IMO.

I'll leave it up to you though. So I'm planning to commit this part of the 
patch as is (I'd really need that committed now). Should be fairly easy to 
merge back the files into a new one later on and will be almost transparent 
for me.

> Just a style note on plasma/CMakeLists.txt:
> 
> +if(KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION)
> +    set(PLASMA_NO_KDEWEBKIT TRUE)
> ...
> +if(NOT PLASMA_NO_KDEWEBKIT)
> +    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)
> 
> I would go for positive logic instead of negative, at least for me this is
> easier to understand:
> 
> +set(PLASMA_WITH_KDEWEBKIT TRUE)
> +if(KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION)
> +    set(PLASMA_WITH_KDEWEBKIT FALSE)
> ...
> +if(PLASMA_WITH_KDEWEBKIT)
> +    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)
> 

Well, I used the FOO_NO* form to be consistent with Qt there.
 
> Also, you could set both the include dir and the link library in one place,
> instead of having two if()s:
> 
> +if(NOT PLASMA_NO_KDEWEBKIT)
> +    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)
> +    set(PLASMA_OPTIONAL_LIBS ${PLASMA_OPTIONAL_LIBS} kdewebkit)
> +endif(NOT PLASMA_NO_KDEWEBKIT)

Indeed modified that in my patch. Improves the readability indeed.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud patron of KDE, http://www.kdab.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20100428/25c68438/attachment-0001.sig