[Marble-devel] Review Request 124822: gsoc: Introduces custom relation editing to placemarks within the AnnotatePlugin

[Dennis Nienhüser]

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



src/lib/marble/osm/OsmRelationEditorDialog.cpp (line 39)
<https://git.reviewboard.kde.org/r/124822/#comment58294>

    Use `tr("Name")` for translation?



src/lib/marble/osm/OsmRelationManagerWidget.cpp (line 59)
<https://git.reviewboard.kde.org/r/124822/#comment58295>

    The usual pattern I know for a modal dialog's lifecycle is that the creator is responsible for deletion, and uses a QPointer for safe deletion (avoids crashes in exceptional cases). This is described here: https://blogs.kde.org/node/3919
    
    The changes required to implement this would be to remove the deleteLater calls in the dialog class and use a QPointer to store the OsmRelationEditorDialog instance.



src/lib/marble/osm/OsmRelationManagerWidget.cpp (line 100)
<https://git.reviewboard.kde.org/r/124822/#comment58296>

    tmp => flags



src/lib/marble/osm/OsmRelationManagerWidget.cpp (line 129)
<https://git.reviewboard.kde.org/r/124822/#comment58297>

    tr() for all Remove and Edit strings in this method?



src/lib/marble/osm/OsmRelationManagerWidget.cpp (line 143)
<https://git.reviewboard.kde.org/r/124822/#comment58298>

    QPointer here as well



src/lib/marble/osm/OsmRelationManagerWidget_p.cpp (line 31)
<https://git.reviewboard.kde.org/r/124822/#comment58299>

    I'd prefer q_, see other review



src/lib/marble/osm/OsmRelationManagerWidget_p.cpp (line 73)
<https://git.reviewboard.kde.org/r/124822/#comment58300>

    mDebug() or Q_ASSERT in addition?



src/lib/marble/osm/OsmRelationManagerWidget_p.cpp (line 85)
<https://git.reviewboard.kde.org/r/124822/#comment58301>

    mDebug() or Q_ASSERT in addition?



src/lib/marble/osm/OsmRelationManagerWidget_p.cpp (line 111)
<https://git.reviewboard.kde.org/r/124822/#comment58302>

    Wrap into tr()?



src/lib/marble/osm/OsmRelationManagerWidget_p.cpp (line 116)
<https://git.reviewboard.kde.org/r/124822/#comment58303>

    mDebug or Q_ASSERT in addition?


- Dennis Nienhüser


On Aug. 19, 2015, 4:07 p.m., Marius Stanciu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124822/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2015, 4:07 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> !!Integrating this ui to the annotate plugin is coming in a separate patch; These are just the classes.
> 
> The widget is used to view and edit relations for any placemark within the annotatePlugin
> A tab will be added to all the editors in the annotatePlugin ( EditPlacemarkDialog, EditPolygonDialog, EditPolylineDialog ).
> This will allow to include any placemark as a member to any existent( or newly created ) relations.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/osm/OsmRelationEditorDialog.h PRE-CREATION 
>   src/lib/marble/osm/OsmRelationEditorDialog.cpp PRE-CREATION 
>   src/lib/marble/osm/OsmRelationManagerWidget.h PRE-CREATION 
>   src/lib/marble/osm/OsmRelationManagerWidget.cpp PRE-CREATION 
>   src/lib/marble/osm/OsmRelationManagerWidget.ui PRE-CREATION 
>   src/lib/marble/osm/OsmRelationManagerWidget_p.h PRE-CREATION 
>   src/lib/marble/osm/OsmRelationManagerWidget_p.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124822/diff/
> 
> 
> Testing
> -------
> 
> created new placemarks, added them to relations. Created relations, modified them, removed them. Seems to work fine.
> 
> 
> Thanks,
> 
> Marius Stanciu
> 
>

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