[Marble-devel] Review Request 124780: gsoc: Introduces the OsmTagEditorWidget

Sat Aug 22 08:02:31 UTC 2015

-----------------------------------------------------------
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
> 
>

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