<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/123579/">https://git.reviewboard.kde.org/r/123579/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 3rd, 2015, 10:10 a.m. UTC, <b>Dennis Nienhüser</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/123579/diff/2/?file=365494#file365494line380" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/data/GeoDataCoordinates.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class GEODATA_EXPORT GeoDataCoordinates</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">380</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="n">QString</span> <span class="nf">lonLatToUTMString</span><span class="p">(</span> <span class="n">qreal</span> <span class="n">lon</span><span class="p">,</span> <span class="n">qreal</span> <span class="n">lat</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">These are not used outside GeoDataCoordinates, right? I'd move them to GeoDataCoordinatesPrivate instead and call them via d->lonLatToUTMString() etc then.</p></pre>
</blockquote>
<p>On May 5th, 2015, 9:09 p.m. UTC, <b>Alejandro García Montoro</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm thinking about reordering all the UTM-related code. My idea is: delete all <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">{lon,lat,lonLat}ToUTMString()</code> I've done and implement four main functions for obtaining the four main values of UTM (zone, latitude band, easting and northing). These four functions would be implemented based on all the mathematical stuff that was placed in GeoDataUTM. Hence, we can move all those mathematical functions to GeoDataCoordinatesPrivate and make them static as you suggested in the other issue.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Futhermore, the four main functions should have two versions: one static and one non-static. My idea es: implement them in GeoDataCoordinatesPrivate as static (maybe something like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">lonLatToZone(qreal lon, qreal lat)</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">lonLatToLatitudeBand(qreal lon, qreal lat)</code>, ...) and then declare the other version in GeoDataCoordinates (they could maybe named just <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">zone()</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">latitudeBand()</code> and so on). The idea behind making a public non-static version is just to give the possibility of obtaining the UTM values from a GeoDataCoordinates object, as it is possible now for the lon/lat units with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">longitude()</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">latitude()</code>. It's not critical but I think it could help to improve the coordinate system support.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Implementing the static versions is mandatory if we want to use them from lonToString and latToString (which are static). However, these two functions need a bigger refactoring as you said in another issue, as this proposal does not solve the real problem: we have to guess the UTM values from just one component of the pair (lon,lat), when we really need both of them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you think this is a good way to advance, I can finish this implementation (I have it nearly done) and update the review request. And, please, feel free to comment anything you does not like.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thank you! :)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sounds all fine to me. Wrt to zone(), latitudeBand() and similar methods I'd rather go for utmZone(), utmLatitudeBand() etc since GeoDataCoordinates::zone seems too generic to me.</p></pre>
<br />
<p>- Dennis</p>
<br />
<p>On May 2nd, 2015, 6:57 p.m. UTC, Alejandro García Montoro wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Alejandro García Montoro.</div>
<p style="color: grey;"><i>Updated May 2, 2015, 6:57 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
marble
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Two different tasks have been accomplished concerning UTM coordinate system:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">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)</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">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.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Still to be fixed</strong>: 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?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Note</strong>: 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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/apps/marble-kde/marble_part.cpp <span style="color: grey">(283ad87a39f91e01c391a4dd8f4c9ddbeef41481)</span></li>
<li>src/apps/marble-qt/QtMainWindow.cpp <span style="color: grey">(c4280c62b4534b7d10387b6895d142920f957bb7)</span></li>
<li>src/lib/marble/geodata/data/GeoDataCoordinates.h <span style="color: grey">(6efabd047f8f3aff63381facb78cb371c0740bf4)</span></li>
<li>src/lib/marble/geodata/data/GeoDataCoordinates.cpp <span style="color: grey">(fe74a2e42db921b152a29ef04246ceaaa16c2325)</span></li>
<li>src/plugins/render/graticule/GraticulePlugin.cpp <span style="color: grey">(a5e6d14a921c66d55cf01e39b16564dedcf6532b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123579/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/05/01/505df756-87f2-47bb-a6c1-d5b52629547a__NorwayScreenshot.png">UTM grid exceptions around Norway</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>