[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: shared lib: conversion-lib in Plasma
From:       Albert Astals Cid <aacid () kde ! org>
Date:       2009-07-23 20:52:17
Message-ID: 200907232252.21687.aacid () kde ! org
[Download RAW message or body]

A Dijous, 23 de juliol de 2009, Carsten Niehaus va escriure:
> Moin moin
>
> With the commits r1001601, r1001602 and r1001603 I moved the lib to
> kde-review.
>
> I hope I did everything correctly... I never moved a lib...
>
> Let the review begin!

* Might be worth Conversion namespace to be named KConversion, KUnitConversion 
?

 * Complex class has empty constructor and destructor defined in a header, 
move it to a cpp

 * Do you really need Unit being a QObject?

 * explicit keyword in Unit constructor it's worthless, only makes sense for 
constructors with 1 parameter
 
 * Might make sense forward declaring KLocalizedString in unit.h instead of 
including it

 * Might make sense forward declaring Unit and QVariant in value.h

 * converter.h is installed and it includes lots of files that are not 
installed rendering the file unusable	

 * About translation, i see you have a Messages.sh that creates 
libconversion.pot but you don't load libconversion catalog in any place inside 
the library, if you have a single point of entry the user of the library is 
guaranteed to use once and only once you should add it there, otherwise you 
should either:
 a) Retire the libconversion catalog when getting inside kdelibs and let the 
kdelibs4.po catalog swallow it all
 b) Mark clearly in the documentation that one needs to insert the 
libconversion catalog when using the library.

Albert


>
> Carsten

[prev in list] [next in list] [prev in thread] [next in thread]