[Marble-devel] Review Request 124570: gsoc: Keeping OsmPlacemarkData synchronized with geometries while editing

[Dennis Nienhüser]

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



src/lib/marble/osm/OsmPlacemarkData.h (line 91)
<https://git.reviewboard.kde.org/r/124570/#comment57626>

    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.



src/lib/marble/osm/OsmPlacemarkData.cpp (line 181)
<https://git.reviewboard.kde.org/r/124570/#comment57620>

    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.



src/lib/marble/osm/OsmPlacemarkData.cpp (line 194)
<https://git.reviewboard.kde.org/r/124570/#comment57621>

    Shouldn't this one check whether newKey equals oldKey? In that case we'd end up removing both.



src/plugins/render/annotate/AreaAnnotation.cpp (line 272)
<https://git.reviewboard.kde.org/r/124570/#comment57622>

    Seems odd to use a comma initializer and only initialize one of the variables.



src/plugins/render/annotate/AreaAnnotation.cpp (line 344)
<https://git.reviewboard.kde.org/r/124570/#comment57623>

    Same here, I'd initialize initialOsmData to nullptr as well.



src/plugins/render/annotate/PolylineAnnotation.cpp (line 391)
<https://git.reviewboard.kde.org/r/124570/#comment57625>

    could osmData be 0 here?


- Dennis Nienhüser


On July 31, 2015, 8:16 p.m., Marius Stanciu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124570/
> -----------------------------------------------------------
> 
> (Updated July 31, 2015, 8:16 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/plugins/runner/osm/translators/OsmDocumentTagTranslator.cpp 3591b6d 
>   src/plugins/runner/osm/OsmObjectManager.cpp 8bebf0c 
>   src/plugins/render/annotate/PolylineAnnotation.cpp 4ae08e8 
>   src/lib/marble/osm/OsmPlacemarkData.h b73a6a0 
>   src/lib/marble/osm/OsmPlacemarkData.cpp 5665ce5 
>   src/plugins/render/annotate/AreaAnnotation.cpp fa3bd16 
> 
> 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/20150803/a638e828/attachment-0001.html>