El Monday 25 January 2016, a les 21:47:34, Albert Astals Cid va escriure: > El Sunday 24 January 2016, a les 16:50:18, Andreas Cord-Landwehr va escriure: > > 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. > > > > 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 > > * 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?) > > * 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? > > Yeah, you should use ki18n in the qml files too.
Ah, it seems Yuri already fixed this. Cheers, Albert > > Cheers, > Albert > > > Other than that, the code quality and licensing information look really > > good. > > > > Cheers, > > Andreas