----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121875/#review73308 -----------------------------------------------------------
Thanks for the patch. It's very nice to see some unit tests! Here are my comments. src/lib/marble/EditTextAnnotationDialog.h <https://git.reviewboard.kde.org/r/121875/#comment50974> No need to repeat the name of the slot here. Also, please describe the behavior of the slot briefly without using "update" and "altitude" or their synomyms. src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50988> Why not add your copyright here, too? src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50975> Is this change really needed? If not, it decreases the "signal to noise" ratio, too. Please revert this change if applicable. src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50976> The surrounding code seems to add whitespace between parenthesis. Please adopt your code to avoid inconsistencies in coding style. Please review and fix all your changes. src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50977> the else belongs on the next line, as per coding style src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50978> Please add whitespace before the last (and only the last) parethesis to respect the surrounding coding style. The SIGNAL and SLOT macros should stay normalized. src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50979> I figure you need to add `break` statements after each case. src/lib/marble/EditTextAnnotationDialog.cpp <https://git.reviewboard.kde.org/r/121875/#comment50987> In case `MeasureUnit` gets extended, this `default:` branch will stop the compiler from complaining about unhandled cases. So in order to make the code more robust, you can factor out the switch statement into a static method which "normalizes" the altitude to meters. In addition, this method could issue a qWarning() if the measure unit wasn't recognized. src/lib/marble/EditTextAnnotationDialog.ui <https://git.reviewboard.kde.org/r/121875/#comment50973> Can you plz revert that single change? I suppose it's not needed for the patch and hence decreases the "signal to noise" ratio. src/lib/marble/MarbleLocale.h <https://git.reviewboard.kde.org/r/121875/#comment50981> Please mark the method as static if possible. Also rename to `meterToTargetUnit`. src/lib/marble/MarbleLocale.h <https://git.reviewboard.kde.org/r/121875/#comment50982> Dito. Please also add a comment. src/lib/marble/MarbleLocale.cpp <https://git.reviewboard.kde.org/r/121875/#comment50980> Please pass those by reference. You seem to depend on the existance of those variables anyway. src/lib/marble/MarbleLocale.cpp <https://git.reviewboard.kde.org/r/121875/#comment50984> Please change to: qWarning() << Q_FUNC_INFO << "Unknown measurement system!"; This will print the error message on stderr in conjunction with the name of the method, including the class name and parameters. src/lib/marble/MarbleLocale.cpp <https://git.reviewboard.kde.org/r/121875/#comment50985> If you used `*targetValue *= ` and `*targetUnit = ` in all case statements, you could spare those two lines, which has another advantage: You could then move the warning after the switch statement and spare the `default:` branch. That way, the compiler will issue a warning if another `MeasureUnit` will be added at some point. src/lib/marble/MarbleLocale.cpp <https://git.reviewboard.kde.org/r/121875/#comment50986> Please move the `return ""` after the switch statement and spare the `default:` branch for the same reasons as above. tests/LocaleTest.cpp <https://git.reviewboard.kde.org/r/121875/#comment50983> Did you try using QCOMPARE here? According to the documentation [1], qFuzzyCompare() is used for floats and doubles (i.e. qreals), so QCOMPARE should work. [1] http://qt-project.org/doc/qt-4.8/qtest.html#QCOMPARE - Bernhard Beschow On Jan. 6, 2015, 5:49 nachm., Illya Kovalevskyy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121875/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2015, 5:49 nachm.) > > > Review request for Marble, Dennis Nienhüser and Torsten Rahn. > > > Repository: marble > > > Description > ------- > > This patch expands MarbleLocale API, so it covers measurement units and also > does add elevation widget to annotation plugin. > > Related GCI task: > http://www.google-melange.com/gci/task/view/google/gci2014/5879562479075328 > > > Diffs > ----- > > src/lib/marble/CMakeLists.txt 807df82 > src/lib/marble/EditTextAnnotationDialog.h a842ade > src/lib/marble/EditTextAnnotationDialog.cpp 877f340 > src/lib/marble/EditTextAnnotationDialog.ui 67256c6 > src/lib/marble/ElevationWidget.ui PRE-CREATION > src/lib/marble/MarbleGlobal.h bd7d63f > src/lib/marble/MarbleLocale.h 0b5414b > src/lib/marble/MarbleLocale.cpp 653f69b > tests/CMakeLists.txt 36ef40b > tests/LocaleTest.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/121875/diff/ > > > Testing > ------- > > Positive > > > Thanks, > > Illya Kovalevskyy > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
