KDiagram libs (KChart, KGantt) in KDE Review

[Friedrich W. H. Kossebau]

Am Montag, 9. Februar 2015, 04:09:59 schrieb Aleix Pol:
> Hi,
> I just went through the cmake code.

Thanks, Aleix.

> Let's see:
> - I see this, what does need to be fixed? > # TODO: fix
> ecm_generate_headers to support camelcase .h files

See https://git.reviewboard.kde.org/r/122317/ , and for related discussion 
also https://git.reviewboard.kde.org/r/122193 

By current feddback in the other RR 122317 seems okay principally, just noone 
has yet ship-it-ed. Looking forward to someone doing so, so KDiagram can soon 
make use of the macro also for KChart (yes, could have also worked-around by 
lowercasing all filenames in the sources, but did not want to change even more 
things than just the namespace).

> - Library targets are usually called KF5*, see KF5Parts or KF5Gantt

Even if not part of KF5 itself? So should any libraries in KDE repos prefix 
their targets like that, because the related ECM macros are used?
I would rather think KF5 prefix should be reserved to libs from KF5, but if 
what you propose is already standard, I will follow.

> - Is this really needed? add_definitions(-DKDAB_NO_UNIT_TESTS). It's
> not very good to compile differently things if there's unit tests and
> not.

Not perfect, agreed. Okay if I add a TODO for now?
(given this is also in existing released code, and fixing might need some 
discussions)

> - some of the definitions in the root CMakeLists.txt files are already
> being defined by KDEFrameworkCompilerSettings. You might want to clean
> them up.

Fixed, 

> - misses a .reviewboardrc file.

Fixed.

> I created a small review request you also want to look into:
> https://git.reviewboard.kde.org/r/122495/

Thanks for that, going to comment on tonight.

Cheers
Friedrich