On Fri, Jan 29, 2016 at 6:47 PM, Sandro Andrade <[email protected]> wrote: > On Sun, Jan 24, 2016 at 12:50 PM, Andreas Cord-Landwehr > <[email protected]> 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();" > > Fixed > >> * an appdata file is missing > > Fixed > >> * 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 > > Fixed > > 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 dependencies? Cheers, Sandro >> >> Other than that, the code quality and licensing information look really good. >> >> Cheers, >> Andreas
