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

[Thibaut Gridel]

> On 2010-07-07 19:01:17, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h, line 75
> > <http://reviewboard.kde.org/r/4285/diff/2/?file=29858#file29858line75>
> >
> >     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?

I kept the old API because I didn't want to force the change everywhere at the same time.
I expect to drop this in next row of patches, because the code is suboptimal to recreate a QVector on demand.
Wrt naming, my change was to turn the return out of a QVector& because I'm rebuilding it... There is some code still using this.


> On 2010-07-07 19:01:17, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h, line 93
> > <http://reviewboard.kde.org/r/4285/diff/2/?file=29858#file29858line93>
> >
> >     Are you sure that you can remove the virtual here?

I think so. The original uncommented use of childPosition was to have a virtual "do nothing" implementation in GeoDataObject, and have that carry for all GeoData.
It seems quite wrong, because only 2 classes (and their inherited) need children, namely GeoDataContainer and GeoDataMultiGeometry. I don't expect a subclass to reimplement the code, so dropped the virtual.


> On 2010-07-07 19:01:17, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h, line 191
> > <http://reviewboard.kde.org/r/4285/diff/2/?file=29869#file29869line191>
> >
> >     Why are you commenting out the private thing? 
> >     That does not look reasonable.
> >     
> >     If there is a reasonable reason, you should comment this.

I did that for the sake of testing into TestGeoData that p() members survived their contorsion through containers. It used to break big time!!! Now that the pointer-based api is in place, and the test proved to work, I am bringing it back to private (and drop test cases).
The same happened in GeoDataFolder.


> On 2010-07-07 19:01:17, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp, line 52
> > <http://reviewboard.kde.org/r/4285/diff/2/?file=29850#file29850line52>
> >
> >     Why don't you use mDebug() there?

Fixed!


> On 2010-07-07 19:01:17, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.cpp, line 107
> > <http://reviewboard.kde.org/r/4285/diff/2/?file=29876#file29876line107>
> >
> >     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.

Will edit in this file to match surrounding code.
However this particular class needs a bigger lifting, because the determination of the root document to resolve styling is a gross hack.
I deemed the patch big enough to stop there ;)


- Thibaut


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


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/04b6d72e/attachment.htm