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

Torsten Rahn tackat at kde.org
Sat May 17 13:16:46 UTC 2014 

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



data/maps/earth/political/political.dgml
<https://git.reviewboard.kde.org/r/118174/#comment40386>

    Could you remove the trailing spaces? :-)
    



data/maps/earth/political/political.dgml
<https://git.reviewboard.kde.org/r/118174/#comment40393>

    As I said before: 
    
    I would like to see a real legend that also has all the items that are shown in the "Plain Map". All of them. :-)



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

    const references don't help here. The whole thing is a string and the "const&" need just get to stripped off. You can just put QString inside the connect method. Will be faster even.



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

    This isn't very descriptive - what would be a better name? datasetPosition or datasetIndex maybe?



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

    Has lineStyle pointed to some other object before? Are we leaking that one? :-)



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

    Same here. Has polyStyle pointed to a different GeoDataPolyStyle object before? Are we leaking it?



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

    Superfluous spaces ...



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

    indentation



src/lib/marble/geodata/data/GeoDataPolyStyle.h
<https://git.reviewboard.kde.org/r/118174/#comment40392>

    doesn't require const.



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

    Usually colors should get stored internally using QColor - or is there a good reason here to store it in a more memory hungry string?



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

    why 99 ? :)



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

    space


- Torsten Rahn


On May 17, 2014, 1:13 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:13 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> 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/20140517/335d5044/attachment-0001.html>

More information about the Marble-devel mailing list