[Marble-devel] Review Request 124672: gsoc: Introducing OsmPresetLibrary class ( refactoring of osm tag presets )

[Dennis Nienhüser]

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



src/lib/marble/geodata/data/GeoDataFeature.h (line 76)
<https://git.reviewboard.kde.org/r/124672/#comment58099>

    Talking about code moves here seems a bit confusing. People can use git's log to find out about that; here we can just write down the current state ("There is an additional tag mapping to ... at ...")



src/lib/marble/osm/OsmPresetLibrary.h (line 47)
<https://git.reviewboard.kde.org/r/124672/#comment58100>

    Since s_visualCategories is private by default it won't appear in doxygen output, so better speak about sth like visual categories



src/lib/marble/osm/OsmPresetLibrary.cpp (line 30)
<https://git.reviewboard.kde.org/r/124672/#comment58103>

    I'd either check here for s_visualCategories.isEmpty() instead of in the functions calling it, or, if you want to spare the additional function call this generates, do a `Q_ASSERT( s_visualCategories.isEmpty() );` here to ensure it is used consistently (think about refactoring).



src/lib/marble/osm/OsmPresetLibrary.cpp (line 187)
<https://git.reviewboard.kde.org/r/124672/#comment58101>

    same as above



src/lib/marble/osm/OsmPresetLibrary.cpp (line 264)
<https://git.reviewboard.kde.org/r/124672/#comment58102>

    I'd check the result of split to avoid a crash on wrong input here. Isn't this problematic already when `else if ( ( category = OsmPresetLibrary::OsmVisualCategory( key ) ) )` is called below?
    
    Generally I wonder if passing key and value separately would be better (feel free to ignore in this review request).


- Dennis Nienhüser


On Aug. 16, 2015, 1:50 p.m., Marius Stanciu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124672/
> -----------------------------------------------------------
> 
> (Updated Aug. 16, 2015, 1:50 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Moved the osm presets from the GeoDataFeature to a separate class. This makes accessing the preset styles much easier and clearer.
> Added additional tags to the list.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/geodata/data/GeoDataFeature.h ea23cd8 
>   src/lib/marble/geodata/data/GeoDataFeature.cpp 4443162 
>   src/lib/marble/geodata/data/GeoDataFeature_p.h 962f0d5 
>   src/lib/marble/osm/CMakeLists.txt 2aa67f7 
>   src/lib/marble/osm/OsmPresetLibrary.h PRE-CREATION 
>   src/lib/marble/osm/OsmPresetLibrary.cpp PRE-CREATION 
>   src/plugins/runner/json/JsonParser.cpp 2c1de39 
>   src/plugins/runner/nominatim-search/OsmNominatimSearchRunner.cpp f21220d 
>   src/plugins/runner/osm/handlers/OsmTagTagHandler.cpp 148ea7d 
> 
> Diff: https://git.reviewboard.kde.org/r/124672/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marius Stanciu
> 
>

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