<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/120004/">https://git.reviewboard.kde.org/r/120004/</a>
</td>
</tr>
</table>
<br />
<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 Calin Cruceru.</div>
<p style="color: grey;"><i>Updated Sept. 4, 2014, 12:09 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">Added 2 tests to show what this patch actually fixes.
Also removed the detach() call from GeoDataFeature::abstractView() because it didn't make any sense since the GeoDataAbstractView pointer is simply assigned in GeoDataFeaturePrivate::operator= (when a new object should be created, and use setParent->this() and delete it in the destructor of the private class, just as GeoDataContainer does).</pre>
</td>
</tr>
</table>
<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;">So I witnessed some time ago that the copy of GeoDataGeometry based classes did not work as expected and I figured out that the main cause were some missing detach() calls in functions returning either non-const references or pointer to some non-const member, which made possible their modification without creating a new *Private instance.</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch adds those missing detach() calls on all GeoDataGeometry and GeoDataFeature based classes (shout me if we are using this technique in other classes so I can take a look there as well).</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is also one important change I thought of in GeoDataPlacemark. Before, in the copy constructor, we used to do p()->m_geometry->setParent( this ) which broke the invariant m_geometry->parent() == this for other. However, there was one more problem: when detach() was called for the first time, the newly created geometry (in GeoDataPlacemarkPrivate) did not have a parent set (it was zeroed) so after a GeoDataPlacemark copy was done, the other geometry's parent would point to the old placemark and the newly created (not immediately, but after the first detach() call) placemark wouldn't have its parent set at all. So, as a workaround, I added p()->m_geometry->setParent(this) after each detach() call in GeoDataPlacemark and removed the call from the copy constructor, so that the invariant is only broken for this, but only until the first detach() call and then each geometry correctly points to its parent.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I also added two new methods to GeoDataFeature. extendedData() and region() did not have both non-const and const getters, but they had a, in case of region, GeoDataRegion& region() const;, which didn't allow the call to detach() but at the same time returned a non-const reference - you don't do something like this usually, do you? The same as above applies here too - tell me if this had some hidden purpose. I can only tell it compiles and all existing tests pass, but I may miss something.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One other minor fix is that the equality operators in GeoDataLink are now const, as they should (this is totally unrelated, but I got over this while testing something - I don't remember exactly what).</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I also removed the virtual void detach() method in GeoDataPoint(). It was only calling GeoDataGeometry::detach() - same as above, tell me if this should stay since it may have some purpose.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">GeoDataTourPrivate now inherits GeoDataFeaturePrivate so that GeoDataTour can use detach() when getting non-const pointer to m_playlist or setting playlist - before, there was no copy constructor in the Private class and the two object pointed to the same GeoDataPlaylist object after copying. I also removed the copy constructor in GeoDataTour and operator= which are no longer needed with the detach() approach.</p>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is one thing I did not find a solution for and may be a problem: the folderList(), placemarkList() and featureList() methods in GeoDataContainare are marked as const but return QVector<GeoDataFeature*> so it may be possible to modify some GeoDataFeature objects through those pointers immediately after a copy of GeoDataContainer, without calling detach() - so it would affect members of the copied object.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/lib/marble/geodata/data/GeoDataContainer.cpp <span style="color: grey">(f0fae00)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature.cpp <span style="color: grey">(9b354a2)</span></li>
<li>src/lib/marble/geodata/data/GeoDataGeometry.cpp <span style="color: grey">(8dfdd52)</span></li>
<li>src/lib/marble/geodata/data/GeoDataMultiGeometry.cpp <span style="color: grey">(7c6d148)</span></li>
<li>src/lib/marble/geodata/data/GeoDataMultiGeometry_p.h <span style="color: grey">(df96074)</span></li>
<li>src/lib/marble/geodata/data/GeoDataMultiTrack.cpp <span style="color: grey">(9cc89bc)</span></li>
<li>src/lib/marble/geodata/data/GeoDataPlacemark.cpp <span style="color: grey">(b2d0304)</span></li>
<li>src/lib/marble/geodata/data/GeoDataPoint.h <span style="color: grey">(401b25f)</span></li>
<li>src/lib/marble/geodata/data/GeoDataPoint.cpp <span style="color: grey">(8c5d9c9)</span></li>
<li>src/lib/marble/geodata/data/GeoDataPoint_p.h <span style="color: grey">(a7c19e0)</span></li>
<li>src/lib/marble/geodata/data/GeoDataPolygon.cpp <span style="color: grey">(b8e0f32)</span></li>
<li>src/plugins/render/annotate/AnnotatePlugin.cpp <span style="color: grey">(b1c8072)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(47b2c68)</span></li>
<li>tests/TestFeatureDetach.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/TestGeometryDetach.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120004/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>