----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124780/#review84175 -----------------------------------------------------------
src/lib/marble/osm/OsmTagEditorWidget.h (line 43) <https://git.reviewboard.kde.org/r/124780/#comment58275> maybe "is most fit" => "fits best" src/lib/marble/osm/OsmTagEditorWidget.cpp (line 70) <https://git.reviewboard.kde.org/r/124780/#comment58276> return QString(); src/lib/marble/osm/OsmTagEditorWidget.cpp (line 76) <https://git.reviewboard.kde.org/r/124780/#comment58277> ... { return; } src/lib/marble/osm/OsmTagEditorWidget.cpp (line 105) <https://git.reviewboard.kde.org/r/124780/#comment58278> +newlines src/lib/marble/osm/OsmTagEditorWidget.cpp (line 142) <https://git.reviewboard.kde.org/r/124780/#comment58279> "" => QString() src/lib/marble/osm/OsmTagEditorWidget.ui (line 85) <https://git.reviewboard.kde.org/r/124780/#comment58293> I wonder if the user visible title should rather be "Related tags" and not "Recommended tags": Not all tags that show up will make sense for a particular item, and the stronger "Recommendation" might lead people to still include them in doubt. src/lib/marble/osm/OsmTagEditorWidget_p.h (line 26) <https://git.reviewboard.kde.org/r/124780/#comment58280> I prefer not using prefix underscore for variable, but rather postfix if needed for disambiguation (i.e. here _q => q_) The reason is that some prefix underscore combinations are reserved: * Reserved in any scope, including for use as implementation macros: * identifiers beginning with an underscore followed immediately by an uppercase letter * identifiers containing adjacent underscores (or "double underscore") * Reserved in the global namespaces: * identifiers beginning with an underscore Taken from http://stackoverflow.com/a/228797 which has some interesting additional information. src/lib/marble/osm/OsmTagEditorWidget_p.h (line 40) <https://git.reviewboard.kde.org/r/124780/#comment58281> Can it be const? Also `generateTagFilter`, `containsAny` and `addPattern` below? src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 30) <https://git.reviewboard.kde.org/r/124780/#comment58282> Wrap into tr() for translation? src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 72) <https://git.reviewboard.kde.org/r/124780/#comment58283> Maybe add a comment that this is not intended for translation, same below for type=multipolygon. Some translators review code for missing tr calls and might wonder about this. src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 124) <https://git.reviewboard.kde.org/r/124780/#comment58285> Alternatively (matter of taste, I find ternary operators more readable for short stuff): ``` itemText << tag.first; itemText << tag.second.isEmpty() ? "<value>" : tag.second; ``` src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 197) <https://git.reviewboard.kde.org/r/124780/#comment58286> nice, but I'm afraid Visual Studio 2010 will choke on this, see http://en.cppreference.com/w/cpp/compiler_support Please use `tags = QStringList() << "amenity=*" << "shop=*" << ...` instead. Same below for each. src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 199) <https://git.reviewboard.kde.org/r/124780/#comment58287> +newlines, same below src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 226) <https://git.reviewboard.kde.org/r/124780/#comment58290> Did you mean: network Same in the comment above src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 282) <https://git.reviewboard.kde.org/r/124780/#comment58291> Did you mean: wheelchair src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 306) <https://git.reviewboard.kde.org/r/124780/#comment58292> Did you mean: abutters src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 334) <https://git.reviewboard.kde.org/r/124780/#comment58288> Please add a shell warning: `mDebug() << "Unexpected OSM tag << tag;` Maybe even a Q_ASSERT, depending how it can be called (programmer error vs. user input) src/lib/marble/osm/OsmTagEditorWidget_p.cpp (line 353) <https://git.reviewboard.kde.org/r/124780/#comment58289> "=" => '=' - Dennis Nienhüser On Aug. 19, 2015, 3:08 p.m., Marius Stanciu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124780/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2015, 3:08 p.m.) > > > Review request for Marble. > > > Repository: marble > > > Description > ------- > > !!! depends on the OsmPresetLibrary patch ( 124672 ) > > If you want to test this separately, add the files manually to the CMakeList > file. > > This widget allows the user to add(remove) tags to placemarks. > The tags can be either selected from a "recommended tags" list, or can be > manually introduced via an "Add custom tag..." item. > > The recommended tag presets are generated based on the current state of the > placemark in the following way: > > - recommendedTags() generates a filter ( using generateTagFilter() ), and > then iterates through the osmPresetLibrary, picking all tags that pass the > filter. > - made an easy, standard way to add criteria to the filter( in case there > are more feasible ones that i have missed ) as you can see in > generateTagFilter(); > > The widget also has a hint function ( suitableTag() ) that returns the tag > that is most fit to represent the visual category of the placemark. > (Currently, the criteria on which that tag is chosen is quite simple ( first > tag ), might have to think of a better one ) > > ! known issue (not really related to this patch): not all visual categories > have their style mapped right in GeoDataFeature.cpp ( eg. > landuse=construction makes the polygon invisible, while landuse=cemetery > works perfectly ) > might fix the styles in a later patch. > > > Diffs > ----- > > src/lib/marble/osm/OsmTagEditorWidget.cpp PRE-CREATION > src/lib/marble/osm/OsmTagEditorWidget.h PRE-CREATION > src/lib/marble/osm/OsmTagEditorWidget.ui PRE-CREATION > src/lib/marble/osm/OsmTagEditorWidget_p.h PRE-CREATION > src/lib/marble/osm/OsmTagEditorWidget_p.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/124780/diff/ > > > Testing > ------- > > testing is done for the next patch ( the ui is not yet integrated in the > annotate plugin ) > > > Thanks, > > Marius Stanciu > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
