KDiagram libs (KChart, KGantt) in KDE Review

[Inge Wallin]

On Monday, February 09, 2015 04:09:59 Aleix Pol wrote:
> On Mon, Feb 9, 2015 at 1:50 AM, Friedrich W. H. Kossebau
> 
> <kossebau at kde.org> wrote:
> > Am Montag, 9. Februar 2015, 00:23:58 schrieb Albert Astals Cid:
> >> El Diumenge, 8 de febrer de 2015, a les 00:29:26, Friedrich W. H.
> >> Kossebau
> >> va
> >> 
> >> escriure:
> >> > Hi,
> >> > 
> >> > Calligra, Massif-Visualizer, KMyMoney (and perhaps more) make use of
> >> > KDAB's
> >> > nice offer of their KDChart in the GPLv2 licensing variant. But as
> >> > there
> >> > are no real public releases of KDChart, all the projects have copied a
> >> > dump of KDChart into their code repositories, updating now and then to
> >> > newer versions of KDChart. Which is anything but perfect.
> >> > 
> >> > To improve things, after some talk with KDABians it was decided to
> >> > create
> >> > a
> >> > KDE repo with a KDE-fied fork of KDChart, based on their latest Qt5ied
> >> > version. So all FLOSS Qt5-based projects have a single place to-go-to
> >> > for
> >> > nice charting. Which would be centrally updated now and then. Still not
> >> > perfect, but an improvement over the current situation.
> >> > To meet the KDE repo requirements, KDAB has also extended the license
> >> > to
> >> > GPLv2+ for this dump :)
> >> 
> >> Thanks KDABians for this.
> >> 
> >> If this is basically a copy&paste from the existing code we're already
> >> shipping i have no objection,
> > 
> > Yes, nearly copy&paste: the copies of KDChart in Calligra & KMyMoney are
> > older (2.4.1, based on Qt4) versions, while the copy of KDChart in
> > Massif-Visualizer matches the version of the KChart lib in KDiagram.
> > 
> >> though you should probably get someone with
> >> CMake knowledge to review (and kill that autogen.py and g++.pri?)
> > 
> > Yes, any volunteers here for reviewing the CMake parts? :) Code should be
> > similar to that of a KF5 tier1 lib.
> > 
> > Cheers
> > Friedrich
> 
> Hi,
> I just went through the cmake code. Let's see:
> - I see this, what does need to be fixed? > # TODO: fix
> ecm_generate_headers to support camelcase .h files
> - Library targets are usually called KF5*, see KF5Parts or KF5Gantt

KDiagram is not a framework and cannot become one until the library is release 
under the LGPL license (right?). So a name starting with KF5 sounds strange to 
me at this point.

> - 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.
> - some of the definitions in the root CMakeLists.txt files are already
> being defined by KDEFrameworkCompilerSettings. You might want to clean
> them up.
> - misses a .reviewboardrc file.
> 
> I created a small review request you also want to look into:
> https://git.reviewboard.kde.org/r/122495/
> 
> Aleix