[Marble-devel] Review Request 118943: Accept Placemarks as children of GeoDataChange

[Dennis Nienhüser]

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



src/lib/marble/geodata/data/GeoDataAnimatedUpdate.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42481>

    Must check for null pointers here. Alternatively, go back to using a non-pointer version of m_update.



src/lib/marble/geodata/data/GeoDataAnimatedUpdate.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42482>

    Let's delete the old update here (unless you switch back to using a non-pointer version). Also m_update should have its parent set in either case:
    
    delete d->m_update;
    d->m_update = update;
    if ( d->m_update ) {
      d->m_update->setParent( this );
    }
    



src/lib/marble/geodata/data/GeoDataUpdate.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42483>

    This is not enough. You must also cover the case where this has m_change set and the other a null pointer, and vice versa. Or use a non-pointer version.
    



src/lib/marble/geodata/data/GeoDataUpdate.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42484>

    Must set parent and, in case of a pointer version, delete the old m_change (see above).



src/lib/marble/geodata/writers/kml/KmlAnimatedUpdateTagWriter.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42485>

    For safety better Q_ASSERT on the nodeType() before using a static_cast



src/lib/marble/geodata/writers/kml/KmlUpdateTagWriter.cpp
<https://git.reviewboard.kde.org/r/118943/#comment42486>

    I'd also check that change()->size() > 0 to avoid ending up with an empty <Update/> tag



tests/data/Tour.kml
<https://git.reviewboard.kde.org/r/118943/#comment42487>

    Isn't that the default value which shouldn't be written?


- Dennis Nienhüser


On June 26, 2014, 6:36 a.m., Sanjiban Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118943/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 6:36 a.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch allows to accept Placemarks as children of GeoDataChange
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/geodata/CMakeLists.txt bac40ac 
>   src/lib/marble/geodata/data/GeoDataAnimatedUpdate.h 5794656 
>   src/lib/marble/geodata/data/GeoDataAnimatedUpdate.cpp 511073c 
>   src/lib/marble/geodata/data/GeoDataChange.h PRE-CREATION 
>   src/lib/marble/geodata/data/GeoDataChange.cpp PRE-CREATION 
>   src/lib/marble/geodata/data/GeoDataUpdate.h ddadee2 
>   src/lib/marble/geodata/data/GeoDataUpdate.cpp ce42922 
>   src/lib/marble/geodata/handlers/kml/KmlAltitudeModeTagHandler.cpp 9827969 
>   src/lib/marble/geodata/handlers/kml/KmlChangeTagHandler.h PRE-CREATION 
>   src/lib/marble/geodata/handlers/kml/KmlChangeTagHandler.cpp PRE-CREATION 
>   src/lib/marble/geodata/handlers/kml/KmlGxAltitudeModeTagHandler.cpp 3a6bf5a 
>   src/lib/marble/geodata/handlers/kml/KmlModelTagHandler.cpp fe2f126 
>   src/lib/marble/geodata/handlers/kml/KmlPlacemarkTagHandler.cpp 110bc40 
>   src/lib/marble/geodata/handlers/kml/KmlUpdateTagHandler.cpp 534749c 
>   src/lib/marble/geodata/parser/GeoDataTypes.h dafcea3 
>   src/lib/marble/geodata/parser/GeoDataTypes.cpp e87b876 
>   src/lib/marble/geodata/writers/kml/KmlAnimatedUpdateTagWriter.h PRE-CREATION 
>   src/lib/marble/geodata/writers/kml/KmlAnimatedUpdateTagWriter.cpp PRE-CREATION 
>   src/lib/marble/geodata/writers/kml/KmlLookAtTagWriter.cpp 4e45daa 
>   src/lib/marble/geodata/writers/kml/KmlPlaylistTagWriter.cpp 86511bb 
>   src/lib/marble/geodata/writers/kml/KmlPointTagWriter.cpp 70eadbc 
>   src/lib/marble/geodata/writers/kml/KmlUpdateTagWriter.cpp fc1ddf6 
>   tests/TestTour.cpp db2d9c9 
>   tests/data/Tour.kml d9fc9ed 
> 
> Diff: https://git.reviewboard.kde.org/r/118943/diff/
> 
> 
> Testing
> -------
> 
> Tested and passed with tests/Tour.kml
> 
> 
> Thanks,
> 
> Sanjiban Bairagya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20140626/2edce3b3/attachment.html>