[Marble-devel] Review Request 118174: Added political map to Marble

Dennis Nienhüser earthwings at gentoo.org
Wed May 21 18:31:01 UTC 2014 

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


Impressive stuff. The comments below mostly address ownership issues.



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40510>

    I agree with Torsten, there's a memory leak here. Note that you're assigning a *copy* of the style in setLineStyle/setPolyStyle. Just create them on the stack to fix it.



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40511>

    Also assigned as a copy and hence leaking



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40502>

    For readability I'd prefer 'iter' instead of 'begin' as variable name



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40503>

    could be const like
    QVector<GeoDataFeature*>::iterator const end
    



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40504>

    Is the expected number of placemarks to be processed huge? Then spare the Q_ASSERT and use a static_cast.



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40505>

    What's the rationale for the exclusion of track and point types?



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40508>

    Memory leak (setPolyStyle/LineStyle takes a copy of the object, not a pointer which it would take ownership of)



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40506>

    Can you Q_ASSERT here that colorsList is not empty? It's guaranteed currently thanks to the !colors.isEmpty() check above, but that's a bit subtle and can easily break on some refactoring.



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40507>

    Is there a guarantee that colorIndex>0 holds?



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40512>

    Memory leak, see above



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40513>

    Memory leak, see above



src/lib/marble/geodata/scene/GeoSceneGeodata.h
<https://git.reviewboard.kde.org/r/118174/#comment40514>

    const QString &
    (I agree with Torsten that parsing the string early would be better)



src/lib/marble/geodata/scene/GeoSceneGeodata.h
<https://git.reviewboard.kde.org/r/118174/#comment40515>

    Should be initialized in the ctor init list



src/plugins/runner/pn2/Pn2Runner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40516>

    Can this be 0 here? Would that be a problem?
    



src/plugins/runner/pn2/Pn2Runner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40517>

    The creation of style here and assignment of them at one place above and another place below look a bit strange to me.
    
    I'm not really sure about the ownership of style here. setStyle of GeoDataFeature takes a pointer, but I found no place where it is deleted, suggesting it is not owned by the feature (still you cannot delete it here).
    



src/plugins/runner/pn2/Pn2Runner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40518>

    Memory leak (copy assigned)



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40519>

    Memory leak (copy assigned)



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40520>

    Memory leak (see above)



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40521>

    That's not a sane conversion. Please check that it is >= 0 and <= 255 and convert like quint8( mapColor ) in that case.
    Is there a meaning for values >-99 and < 0?



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40523>

    (not a memory leak, placemark takes ownership)



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40522>

    Personally I find
    isRingClockwise = crossProduct > 0;
    to be easier readable.



src/plugins/runner/shp/ShpRunner.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40524>

    (not a memory leak, multigeometry takes ownership)



tools/shp2pn2/shp2pn2.cpp
<https://git.reviewboard.kde.org/r/118174/#comment40526>

    Please define directly above when declaring the variable. The existing variables above are a bad example.


- Dennis Nienhüser


On May 17, 2014, 1:21 p.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118174/
> -----------------------------------------------------------
> 
> (Updated May 17, 2014, 1:21 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser, Torsten Rahn, and Thibaut Gridel.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch contains the following cahnges:
> 1. Modified pn2 format, shp runner, pn2 runner and shp2pn2 tool so that we can parse mapcolor field from shapefile and store it in pn2 format.
> 2. Added support to parse new attribute colorMap in <brush> tag for .dgml files.
> 3. Modified GeoDataPolyStyle to add one more data member to store this mapcolor( or color index ) value.
> 4. Changed MarbleModel
>    4.1 not to parse the same sourcefile again on theme change in case the GeoSceneGeoData objects in two themes differ only by <brush> and 
>        <pen>. Just the assign new style to already parsed file. ( so patch https://git.reviewboard.kde.org/r/117978/ can be ignored ).
>    4.2 Handle the case when <brush> has colorMap attribute and assign the placemarks appropriate style based on its color index( or mapcolor).
> 5. Added a new theme file political.dgml.
> 
> 
> Diffs
> -----
> 
>   data/maps/earth/political/political-preview.png PRE-CREATION 
>   data/maps/earth/political/political.dgml PRE-CREATION 
>   data/naturalearth/ne_10m_admin_0_boundary_lines_land.pn2 7ab70bb 
>   data/naturalearth/ne_10m_rivers_lake_centerlines.pn2 3ffcf84 
>   data/naturalearth/ne_50m_admin_0_boundary_lines_land.pn2 25406b0 
>   data/naturalearth/ne_50m_admin_0_boundary_lines_maritime_indicator.pn2 58f9538 
>   data/naturalearth/ne_50m_admin_0_breakaway_disputed_areas.pn2 c1e317b 
>   data/naturalearth/ne_50m_admin_0_countries.pn2 PRE-CREATION 
>   data/naturalearth/ne_50m_admin_0_pacific_groupings.pn2 33e2112 
>   data/naturalearth/ne_50m_admin_1_states_provinces_lines.pn2 637c2ea 
>   data/naturalearth/ne_50m_antarctic_ice_shelves_lines.pn2 83c859a 
>   data/naturalearth/ne_50m_antarctic_ice_shelves_polys.pn2 8201bbc 
>   data/naturalearth/ne_50m_coastline.pn2 91bddc9 
>   data/naturalearth/ne_50m_glaciated_areas.pn2 1c58348 
>   data/naturalearth/ne_50m_lakes.pn2 af0859c 
>   data/naturalearth/ne_50m_lakes_historic.pn2 d5272db 
>   data/naturalearth/ne_50m_land.pn2 ed343a3 
>   data/naturalearth/ne_50m_playas.pn2 5eeadc8 
>   data/naturalearth/ne_50m_rivers_lake_centerlines.pn2 5298ed6 
>   data/naturalearth/ne_50m_urban_areas.pn2 ad80107 
>   src/lib/marble/MarbleModel.h 3470b0e 
>   src/lib/marble/MarbleModel.cpp 83493c3 
>   src/lib/marble/geodata/data/GeoDataPolyStyle.h d38e07e 
>   src/lib/marble/geodata/data/GeoDataPolyStyle.cpp d7377c0 
>   src/lib/marble/geodata/handlers/dgml/DgmlAttributeDictionary.h 9c66830 
>   src/lib/marble/geodata/handlers/dgml/DgmlAttributeDictionary.cpp b250dc1 
>   src/lib/marble/geodata/handlers/dgml/DgmlBrushTagHandler.cpp 8460c4c 
>   src/lib/marble/geodata/scene/GeoSceneGeodata.h 7c9c462 
>   src/lib/marble/geodata/scene/GeoSceneGeodata.cpp 6bd1885 
>   src/plugins/runner/pn2/Pn2Runner.cpp 877abe1 
>   src/plugins/runner/shp/ShpRunner.cpp cad23e9 
>   tools/shp2pn2/shp2pn2.cpp 4e0dc7b 
> 
> Diff: https://git.reviewboard.kde.org/r/118174/diff/
> 
> 
> Testing
> -------
> 
> Works fine on my system.
> 
> 
> File Attachments
> ----------------
> 
> political-preview1.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/17/01c743cf-e12b-4046-bdf4-076a4f2600d4__political-preview1.png
> political-preview2.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/17/2ddfe907-2fa0-4bca-be9e-26196f918d39__political-preview2.png
> naturalearth.zip
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/17/b1142299-b618-4fb8-b23d-91e66d5d2156__naturalearth.zip
> 
> 
> Thanks,
> 
> Abhinav Gangwar
> 
>

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

More information about the Marble-devel mailing list