Minuet (music education software) moved to kdereview
sandroandrade at kde.org
Fri Jan 29 21:53:23 GMT 2016
On Fri, Jan 29, 2016 at 6:47 PM, Sandro Andrade <sandroandrade at kde.org> wrote:
> On Sun, Jan 24, 2016 at 12:50 PM, Andreas Cord-Landwehr
> <cordlandwehr at kde.org> wrote:
>> Hi Sandro, it is always great when such a cool application lands in KDE Edu.
>> I just made a first and rough (since I do not have all dependencies yet to
>> really compile and test it) code review.
> Hi Andreas,
> Thanks for reviewing :) Please find below some comments/questions:
>> Here a some minors I noticed:
>> * the application does not link against KCrash (which is needed for DrKonqi);
>> also in the main.cpp there should be a call to "KCrash::initialize();"
>> * an appdata file is missing
>> * in the main CMakeLists.txt, for KF5 there is no minimum version and for ECM
>> the minimum version should probably be higher than 1.0.0
> Regarding Minuet versioning itself: should I use the automatic version
> numbering from release script or should I start from a 1.0.0 release?
> Also, where are those release scripts stored (some repository)?
>> * it looks strange to me that in minuet/cmake/ there are Config-files for the
>> 3rd-party library drumstick. My understanding was that such Config files should
>> only be shipped with the respective library (maybe someone with a deeper
>> CMake-knowledge can comment?)
> As Albert pointed out, drumstick provides no CMake related files :) so
> we should handle that by ourselves.
>> * qml-file-localization: It looks strange to me that you are mixing two
>> different localization systems with KI18n (in all C++ and UI files) and qsTr in
>> all QML files. Since I am not familiar with qsTr, I cannot really comment on
>> how it works and if everything is setup correctly; at least Messages.sh do not
>> process qml files and hence do not extract strings from them, but that might
>> only be necessary for Ki18n?
This also has now being fixed. I've tried the use of pt_BR
translations and everything seems to work fine.
Question: Minuet uses TiMidity++ and freepats as run-time
dependencies. Should I improve CMakeLists.txt to detect such
installations (they aren't libraries, maybe some ugly check is
required) as a way to hint packagers to add those as Minuet's
>> Other than that, the code quality and licensing information look really good.
More information about the kde-core-devel