[Marble-devel] Review Request 120004: Solve copy problems by adding missing detach() calls. Other minor fixes.

[Thibaut Gridel]

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

Ship it!


Excellent!

you may add some tests for const access to ensure that we get what we expect before detach happened.

- Thibaut Gridel


On sep. 4, 2014, 7:01 matin, Calin Cruceru wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120004/
> -----------------------------------------------------------
> 
> (Updated sep. 4, 2014, 7:01 matin)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> 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.
> 
> * 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).
> 
> * 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.
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   tests/TestGeometryDetach.cpp PRE-CREATION 
>   tests/TestFeatureDetach.cpp PRE-CREATION 
>   tests/CMakeLists.txt 47b2c68 
>   src/plugins/render/annotate/AnnotatePlugin.cpp b1c8072 
>   src/lib/marble/geodata/data/GeoDataPolygon.cpp b8e0f32 
>   src/lib/marble/geodata/data/GeoDataPoint_p.h a7c19e0 
>   src/lib/marble/geodata/data/GeoDataPoint.cpp 8c5d9c9 
>   src/lib/marble/geodata/data/GeoDataPoint.h 401b25f 
>   src/lib/marble/geodata/data/GeoDataPlacemark.cpp b2d0304 
>   src/lib/marble/geodata/data/GeoDataMultiTrack.cpp 9cc89bc 
>   src/lib/marble/geodata/data/GeoDataMultiGeometry_p.h df96074 
>   src/lib/marble/geodata/data/GeoDataMultiGeometry.cpp 7c6d148 
>   src/lib/marble/geodata/data/GeoDataFeature.cpp 9b354a2 
>   src/lib/marble/geodata/data/GeoDataGeometry.cpp 8dfdd52 
>   src/lib/marble/geodata/data/GeoDataContainer.cpp f0fae00 
> 
> Diff: https://git.reviewboard.kde.org/r/120004/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Calin Cruceru
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20140905/8a8ba552/attachment.html>