<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104990/">http://git.reviewboard.kde.org/r/104990/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I wonder if it is a good idea to "fix" wrong usage of our API because "fixing" introduces the potential of surprises. I'd expect the API to return garbage if I pass garbage (garbage-in-garbage-out principle) rather than having it perform arbitrary changes in the background. After all, fixing usually depends on the context and in addition, there are more efficient methods to ensure intended behavior of corner cases such as unit tests.
Moreover, I don't like the flags approach which adds even more complexity to our API. In particular, it'll be hard to predict the behavior of instances of GeoDataCoordinates ("does the instance normalize or not?"). I think that a isNormalized() method, perhaps in conjunction with Q_ASSERT(), could catch wrong usage of our API and will not add additional complexity.</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On May 19th, 2012, 9:15 a.m., Dennis Nienhüser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Dennis Nienhüser.</div>
<p style="color: grey;"><i>Updated May 19, 2012, 9:15 a.m.</i></p>
<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;">We don't officially support non-normalized coordinates (lon outside -180...180 degree, lat outside -90..90 degree). To avoid potential pitfalls when developers using Marble as a library ignore that, it might make sense to normalize coordinates behind the scenes. The patch adds that for GeoDataCoordinates, which should cover most cases. It's controlled by a flag (using QFlags for future extensibility) which defaults to normalization.</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;">The major concern should be performance.
Loading a long route (2500 km, 35.000 coordinates), the performance was pretty much identical (~200 ms) for several runs with and without normalization
The startup time for parsing the various files at startup increases slighty from around 1200 ms on my system to around 1300 ms. We might reduce this difference when we remove now unnecessary calls to normalizeLonLat e.g. in GeoDataLatLonBox::fromString. It could even lead to performance improvements later on.
A callgrind log measuring the start, initialization and shutdown of Marble shows that 0.07% of the time (110.000 calls) was spent in normalizeLonLat(). From that 50.000 calls could be spared when removing the normalization in GeoDataLatLonBox::fromString. In summary, the runtime overhead seems to be small.
</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/lib/geodata/data/GeoDataCoordinates.h <span style="color: grey">(8929430)</span></li>
<li>src/lib/geodata/data/GeoDataCoordinates.cpp <span style="color: grey">(c1563a6)</span></li>
<li>src/lib/geodata/data/GeoDataCoordinates_p.h <span style="color: grey">(14c5164)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104990/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>