----------------------------------------------------------- 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 > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
