[Marble-devel] Review Request 124570: gsoc: Keeping OsmPlacemarkData synchronized with geometries while editing
Marius Stanciu
stanciumarius94 at gmail.com
Sun Aug 16 16:55:07 UTC 2015
> On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/osm/OsmPlacemarkData.cpp, line 181
> > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388971#file388971line181>
> >
> > Did you consider moving from the QHash to a QVector (where the keys are implicitly given as (0...vector.size()-1) now that int's are used keys? At least here this would be much easier.
> > Just wondering, I'm fine with keeping it like this if you prefer.
I actually considered moving to QVector, but that imposed a bunch of restrictions i didn't want: boundaries had to be inserted in order, all boundaries had to be inserted ( no boundary can be missing at any given time ), there's no negative index ( -1 for outerboundary ). There were a few more restrictions i encountered using Qvector i can't really rember right now.
I'd like to keep the Qhash for the moment.
> On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/osm/OsmPlacemarkData.h, line 91
> > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388970#file388970line91>
> >
> > For the five `*Reference` method variants here I'd prefer to distinguish them by their names also, not just by the parameter:
> >
> > containsReference(Coordinates) => containsNodeReference
> > containsReference(int) => containsMemberReference
> > removeReference(Coordinates) => removeNodeReference
> > removeReference(int) => removeMemberReference
> > changeReference(GeoDataCoordinates,GeoDataCoordinates) => changeNodeReference(GeoDataCoordinates,GeoDataCoordinates)
> >
> > If you prefer shorter names, we could also do referencesNode, referencesMember, removeNode, removeMember, moveNode.
> >
> > Ctrl+Shift+R does a quick rename refactoring in QtCreator.
moved to a separate patch
> On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/osm/OsmPlacemarkData.cpp, line 194
> > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388971#file388971line194>
> >
> > Shouldn't this one check whether newKey equals oldKey? In that case we'd end up removing both.
moved to separate patch
> On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote:
> > src/plugins/render/annotate/AreaAnnotation.cpp, line 272
> > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388972#file388972line272>
> >
> > Seems odd to use a comma initializer and only initialize one of the variables.
the initialOsmData is not a pointer, that's why i didn't initialize it. I separated the declaration from the initialization into 2 lines
- Marius
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124570/#review83373
-----------------------------------------------------------
On Aug. 16, 2015, 4:55 p.m., Marius Stanciu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124570/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2015, 4:55 p.m.)
>
>
> Review request for Marble.
>
>
> Repository: marble
>
>
> Description
> -------
>
> In order to achieve this, the following has been done:
>
> Wherever geometries are altered within the annotate plugin, I also modified the osmPlacemarkData objects accordingly
> Eg: -a node is deleted =======> a reference within the osmData for that node is removed
> -a node is moved =======> the reference of that node within the osmData is changed to the new coordinates
>
> I also realised there's one extra case at writing time i haven't thought of:
> Placemarks might have semi-initialized osmData.
> Eg. a polygon is loaded from an ".osm" file with complete OsmData. In the editor, a new innerBoundary is added. This new
> boundary does not yet have osmData ).
>
> In order to take this case in consideration, I modified the OsmObjectManager to check for such a case.
>
>
> Diffs
> -----
>
> src/lib/marble/osm/OsmPlacemarkData.h b73a6a0
> src/lib/marble/osm/OsmPlacemarkData.cpp 5665ce5
> src/plugins/render/annotate/AreaAnnotation.cpp fa3bd16
> src/plugins/render/annotate/PolylineAnnotation.cpp 4ae08e8
> src/plugins/runner/osm/OsmObjectManager.cpp 8bebf0c
> src/plugins/runner/osm/translators/OsmDocumentTagTranslator.cpp 3591b6d
>
> Diff: https://git.reviewboard.kde.org/r/124570/diff/
>
>
> Testing
> -------
>
> Tested every case: loading placemarks from files, creating placemarks, modifying loaded placemarks in every way that i thought of. The results are okay.
> There are a lot of ways to modify placemarks, I might have skipped a few please report any bugs :)
>
>
> Thanks,
>
> Marius Stanciu
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150816/cf6ca8e5/attachment-0001.html>
More information about the Marble-devel
mailing list