[Marble-devel] Review Request 123579: UTM grid and coordinates fixed

Dennis Nienhüser earthwings at gentoo.org
Sun May 3 10:10:12 UTC 2015 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123579/#review79789
-----------------------------------------------------------



src/lib/marble/geodata/data/GeoDataCoordinates.h (line 380)
<https://git.reviewboard.kde.org/r/123579/#comment54656>

    These are not used outside GeoDataCoordinates, right? I'd move them to GeoDataCoordinatesPrivate instead and call them via d->lonLatToUTMString() etc then.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 589)
<https://git.reviewboard.kde.org/r/123579/#comment54657>

    s_ is a (sometimes used) prefix for static variables
    m_ is a (strongly suggested) prefix for class member variables
    
    sm_a is neither static nor a class member. So it would be just a or g_a (g_ is sometimes used for global).
    
    However, I'd rather move all of the content of the GeoDataUTM namespace to GeoDataCoordinatesPrivate (without a special namespace). Then you can declare them static and keep the sm_ prefix as it will be correct there. The methods can become static methods in GeoDataCoordinatesPrivate.
    
    For non-local variables, a and b are way to generic names. We need something longer, more descriptive. sm_EccSquared is ok. UTMScaleFactor should become sm_utmScaleFactor (all moved to GeoDataCoordinatesPrivate)



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 658)
<https://git.reviewboard.kde.org/r/123579/#comment54653>

    Can you use a QPointF instead of an array for xy? The signature would become
    
    `QPointF latLonToUTMXY( qreal lon, qreal lat, qreal zone );`
    
    Same for `mapLatLonToXY` above.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 664)
<https://git.reviewboard.kde.org/r/123579/#comment54654>

    Declaring variables at the top of a method is bad practice. Cons:
    - you cannot initialize them properly (with a meaningful value)
    - if you do not initialize them (like here), there's danger of using them uninitialized by accident before assigning a value
    - you cannot mark them const
    
    So instead of
    `epsilon = (315.0 * qPow (n, 4.0) / 512.0);`
    use
    `qreal const epsilon = (315.0 * qPow (n, 4.0) / 512.0);`
    and so on.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 760)
<https://git.reviewboard.kde.org/r/123579/#comment54655>

    Please be aware of https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Variable_declaration. In particular it wants variables to start with a lowercase letter, have meaningful names, no abbreviations and no single character names except for counter variables.
    
    I see the point of ignoring that to some extent in methods that implement some mathematical formula. So I'm not asking to change something here in particular, but instead ask to think about variable names in general when working on code like this.
    
    Another neat trick is to use arrays/vectors/matrices when you need to declare a lot of "similar" variables. In this case it might be better to replace l3coef, l4coef, ..., l8coef with 
    
    `QVector<qreal> coef(9);`
    `coef[3] = 1.0 - t2 + nu2;`
    ...
    
    such that l3coef becomes coef[3], l4coef becomes coef[4] and so on.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 1191)
<https://git.reviewboard.kde.org/r/123579/#comment54652>

    Isn't that the point where we need to admit that our API is wrong and we need to refactor? For UTM lat/lon => string needs both lat/lon values, whereas the older units can convert directly from a single lat or lon, respectively. So we shouldn't guess (and display wrong stuff somewhere), but instead adjust our methods. In particular hide this one and latToString.
    
    The issue seems to exist in the current code already, but I'd like to see a // @FIXME comment at least, ideally pointing to a (to be created) bug report.
    I'm concerned about classes like MarbleWidgetPopupMenu and CurrentLocationWidget who call this method.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 1440)
<https://git.reviewboard.kde.org/r/123579/#comment54658>

    Generally it's better to use QString's formatting methods for both readability and performance reasons. E.g. compare
    
    `QString const pointString = "(" + QString::number(point.x(), 'f', 2) + ", " + QString::number(point.y(), 'f', 2) + ")";`
    to
    `QString const pointString = QString("(%1, %2)").arg(point.x(), 'f', 2).arg(point.y(), 'f', 2);`
    both will result in pointString having a value like `(3, 4)` but IMHO the second reads better. Similarly the latter is better when UI translations are necessary.



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 1466)
<https://git.reviewboard.kde.org/r/123579/#comment54650>

    the the => the



src/lib/marble/geodata/data/GeoDataCoordinates.cpp (line 1509)
<https://git.reviewboard.kde.org/r/123579/#comment54651>

    uninitilized => uninitialized


- Dennis Nienhüser


On May 2, 2015, 6:57 p.m., Alejandro García Montoro wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123579/
> -----------------------------------------------------------
> 
> (Updated May 2, 2015, 6:57 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Two different tasks have been accomplished concerning UTM coordinate system:
> 
> 1. The UTM grid rendering has been fixed. Now the grid and labels are drawn as expected (see http://upload.wikimedia.org/wikipedia/commons/e/ed/Utm-zones.jpg for comparison)
> 2. Some UTM functions have been added in order to properly calculate and display the coordinates on the status bar. Those functions have been wrapped in the new GeoDataUTM namespace inside GeoDataCoordinates.cpp If you prefer to see the functions integrated in the GeoDataCoordinates class, please, let me know.
>  
> **Still to be fixed**: The UTM string showed in the status bar is too long for the text field in which it is shown. I have not been able to change its size, as I have not found the relevant .ui file; where in the code should I check to change that?
> 
> **Note**: The lat/lon <-> UTM conversion algorithm has been adapted from this JavaScript code http://home.hiwaay.net/~taylorc/toolbox/geography/geoutm.html. The author says in the page "The JavaScript source code in this document may be copied and reused without restriction", and I have linked the website in the code. However, as I am not sure about all the licenses and authoring stuff, I would like you to review it.
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-kde/marble_part.cpp 283ad87a39f91e01c391a4dd8f4c9ddbeef41481 
>   src/apps/marble-qt/QtMainWindow.cpp c4280c62b4534b7d10387b6895d142920f957bb7 
>   src/lib/marble/geodata/data/GeoDataCoordinates.h 6efabd047f8f3aff63381facb78cb371c0740bf4 
>   src/lib/marble/geodata/data/GeoDataCoordinates.cpp fe74a2e42db921b152a29ef04246ceaaa16c2325 
>   src/plugins/render/graticule/GraticulePlugin.cpp a5e6d14a921c66d55cf01e39b16564dedcf6532b 
> 
> Diff: https://git.reviewboard.kde.org/r/123579/diff/
> 
> 
> Testing
> -------
> 
> Changes tested in master branch @ 1st May.
> Grid and coordinates work well (except for the displaying issue explained above) and are consistent with the UTM coordinate system.
> 
> 
> File Attachments
> ----------------
> 
> UTM grid exceptions around Norway
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/05/01/505df756-87f2-47bb-a6c1-d5b52629547a__NorwayScreenshot.png
> 
> 
> Thanks,
> 
> Alejandro García Montoro
> 
>

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

More information about the Marble-devel mailing list