[Marble-devel] Review Request 111810: GeoJSON parser adapted to match the standard.

[Bernhard Beschow]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111810/#review36919
-----------------------------------------------------------


First of all, I tested your patch by adopting the vectorosm map theme. It works very well! Here are my comments on your code:


src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/111810/#comment27227>

    Please introduce cache variables with descriptive names whenever calling QScriptEngine::evaluate().



src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/111810/#comment27224>

    Why do an upcast here...



src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/111810/#comment27225>

    and do a downcast here...



src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/111810/#comment27226>

    and here? There are other cases like this in your code. Please simplify everywhere.



src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/111810/#comment27223>

    It looks like you're leaking all geometries and placemarks if this condition isn't met. Please fix.


- Bernhard Beschow


On July 31, 2013, 1:07 p.m., Ander Pijoan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111810/
> -----------------------------------------------------------
> 
> (Updated July 31, 2013, 1:07 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> -Changed Marble's GeoJSON parser (which was a test for just matching Kothic's GeoJSON like format) to match the exact GeoJSON standard : 
> 
> http://www.geojson.org/geojson-spec.html#id3
> 
> -Supported geometry types :
> 
> Point
> MultiPoint
> LineString
> MultiLineString
> Polygon
> MultiPolygon
> 
> -Not supported geometry types:
> 
> GeometryCollection
> 
> -Geometry properties are compared with GeoDataFeature's OsmVisualCategory to see if any of the properties matches a style to give to the geometry. If not it will be shown as a black line.
> 
> 
> Diffs
> -----
> 
>   src/plugins/runner/json/JsonParser.h 4fb072c 
>   src/plugins/runner/json/JsonParser.cpp 86cf66b 
>   src/plugins/runner/json/JsonPlugin.cpp e7ff770 
>   src/plugins/runner/json/JsonRunner.cpp 2098e1f 
> 
> Diff: http://git.reviewboard.kde.org/r/111810/diff/
> 
> 
> Testing
> -------
> 
> Some test files :
> 
> http://tile.openstreetmap.us/vectiles-skeletron/12/1206/1539.json (this file will matches road styles)
> 
> http://tile.openstreetmap.us/vectiles-buildings/12/1206/1539.json (this file matches some other geometry types)
> 
> http://tile.openstreetmap.us/vectiles-water-areas/12/1206/1539.json (this file matches some other multi geometry types)
> 
> 
> Thanks,
> 
> Ander Pijoan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20130801/5f184ba7/attachment.html>