[Marble-devel] Review Request 124231: gsoc: Adds the OsmPlacemarkData class

[Dennis Nienhüser]

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



src/lib/marble/osm/OsmPlacemarkData.h (line 48)
<https://git.reviewboard.kde.org/r/124231/#comment56342>

    typo, assigns



src/lib/marble/osm/OsmPlacemarkData.h (line 62)
<https://git.reviewboard.kde.org/r/124231/#comment56343>

    Qt API design suggests naming this isVisible



src/lib/marble/osm/OsmPlacemarkData.h (line 85)
<https://git.reviewboard.kde.org/r/124231/#comment56346>

    I kinda dislike using the names nd and ref in the code directly. In the osm XML it makes sense to use abbreviations to reduce the file size, but KDE code style suggests not to do that in C++. So here I'd go for node and reference when naming variables and methods.



src/lib/marble/osm/OsmPlacemarkData.h (line 94)
<https://git.reviewboard.kde.org/r/124231/#comment56348>

    should either be
    const OsmPlacemarkData &ref(...)
    or
    OsmPlacemarkData ref(...)



src/lib/marble/osm/OsmPlacemarkData.h (line 131)
<https://git.reviewboard.kde.org/r/124231/#comment56351>

    int
    
    I'd use the same types as https://wiki.openstreetmap.org/wiki/Elements#Common_attributes suggests. Otherwise they'd have to be validated when writing.



src/lib/marble/osm/OsmPlacemarkData.h (line 132)
<https://git.reviewboard.kde.org/r/124231/#comment56353>

    int



src/lib/marble/osm/OsmPlacemarkData.h (line 133)
<https://git.reviewboard.kde.org/r/124231/#comment56352>

    int



src/lib/marble/osm/OsmPlacemarkData.h (line 134)
<https://git.reviewboard.kde.org/r/124231/#comment56350>

    Should be a bool, me thinks.



src/lib/marble/osm/OsmPlacemarkData.h (line 136)
<https://git.reviewboard.kde.org/r/124231/#comment56349>

    What about using QDateTime? It should have builtin support for the osm timestamp string format, so conversion is easy and does a validation directly.


- Dennis Nienhüser


On July 2, 2015, 4 p.m., Marius Stanciu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124231/
> -----------------------------------------------------------
> 
> (Updated July 2, 2015, 4 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> See comment on the header on what the class is for:
> 
> A new folder lib/marble/osm was created because more sources will come in the following patches. Those sources are needed by both the Osm and Annotate plugins, but also the lib itself.
> 
> I have tried several approaches for managing osm data, and this seemed the most feasible.
> I have tried making it polymorphic, ( OsmWayData: OsmPlacemarkData and OsmRelationData: OsmPlacemarkData )
> but that approach really made things more complicated and seemed a bit like overdesigning for a simple data container class this is. The empty QHashes barely use any memory, so it should be great.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/CMakeLists.txt 8b36d08 
>   src/lib/marble/geodata/data/GeoDataCoordinates.h 7b291cc 
>   src/lib/marble/osm/CMakeLists.txt PRE-CREATION 
>   src/lib/marble/osm/OsmPlacemarkData.h PRE-CREATION 
>   src/lib/marble/osm/OsmPlacemarkData.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124231/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marius Stanciu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150702/6e894ca3/attachment-0001.html>