----------------------------------------------------------- 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 > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
