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