[Marble-devel] Review Request 121875: Tweaking MarbleLocale & adding elevation widget to AnnotationPlugin

[Bernhard Beschow]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150106/51a799f8/attachment-0001.html>