[Marble-devel] Review Request: GeoData: showcase test for pointer-based GeoData

[Bastian Holst]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4285/#review6412
-----------------------------------------------------------


The general direction of the patch looks fine,
but I get a linking error:

Linking CXX shared module ../../../../lib/GeoRendererPlugin.so                              
CMakeFiles/GeoRendererPlugin.dir/GeoRendererView.o: In function `Marble::GeoRendererView::renderIndex(QModelIndex&)':
/home/bastian/devel/kde/marble/src/plugins/render/geodata/GeoRendererView.cpp:109: undefined reference to `Marble::GeoDataTypes::GeoDataDocumentType'
collect2: ld returned 1 exit status

There also some minor issues I see ...:


/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.h
<http://reviewboard.kde.org/r/4285/#comment6132>

    No, it's a GeoDataTreeModel



/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp
<http://reviewboard.kde.org/r/4285/#comment6133>

    Why don't you use mDebug() there?



/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h
<http://reviewboard.kde.org/r/4285/#comment6134>

    I'm not sure here.
    What is the reason for still having the features() method?
    If we really need this, we should do a documentation of both and clearly describe the difference.
    
    Is this the typical Qt naming scheme?



/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h
<http://reviewboard.kde.org/r/4285/#comment6135>

    Are you sure that you can remove the virtual here?



/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h
<http://reviewboard.kde.org/r/4285/#comment6136>

    Why are you commenting out the private thing? 
    That does not look reasonable.
    
    If there is a reasonable reason, you should comment this.



/trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.cpp
<http://reviewboard.kde.org/r/4285/#comment6137>

    Again a minor thing.
    You don't use spaces inside the parenthesis and the opening brace should be in the same line with the if.


- Bastian


On 2010-06-27 21:46:09, Thibaut Gridel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4285/
> -----------------------------------------------------------
> 
> (Updated 2010-06-27 21:46:09)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> providing a (naive) pointer-based api for GeoDataContainer, and test case which shows early breaks...
> 
> Toggle between the two "document.append(folder);" shows how brittle the api still is.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/KmlFileViewItem.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/KmlFileViewItem.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleGeometryModel.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleGeometryModel.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer_p.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFolder.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataGeometry.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry_p.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDocumentTagWriter.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererPlugin.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.h 1143093 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.cpp 1143093 
>   /trunk/KDE/kdeedu/marble/tests/TestGeoData.cpp 1143093 
> 
> Diff: http://reviewboard.kde.org/r/4285/diff
> 
> 
> Testing
> -------
> 
> Run TestGeoData :(
> 
> 
> Thanks,
> 
> Thibaut
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20100707/89a36b7a/attachment-0001.htm