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? > > Other than that, the code quality and licensing information look really good. > > Cheers, > Andreas
