[Marble-devel] Re: Review Request: Refactor MarblePlacemarkModel. Use GeoDataTreeModel as reference model

Dennis Nienhüser earthwings at gentoo.org
Sat Dec 11 21:48:04 CET 2010 

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

Ship it!


Didn't spot anything going wrong when testing it on the N900. And it's indeed faster.
See the comments on IRC (addressing the patch series on gitorious).


- Dennis


On 2010-12-04 00:14:25, Thibaut Gridel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6040/
> -----------------------------------------------------------
> 
> (Updated 2010-12-04 00:14:25)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Please review this code refactor in the model and geodata dept.
> 
> The goal is to use the tree model and proxies above geodata documents, and deprecate the awkward MarblePlacemarkModel.
> 
> For this purpose, the following is done:
> - the datafacade does not keep a MarblePlacemarkModel just to have a flat list of placemark, it uses the stolen KDescendantsProxyModel above the treemodel.
> - the users of the placemarkModel: PlacemarkLayout and NavigationWidget, keep their own sorting and filtering proxy models with them (unclutters MarbleModel)
> - Where placemarkModel::data() was abused to retrieve placemark specific data, a pointer to a placemark is retrieved and direct methods of the placemark are used (lot faster too). Only the basic roles which have a filtering interest (name, coordinates, popularity...) should stay.
> - MarbleModel, Map and Widget had a centerOn(QModelIndex) without referring to which model those indexes were. There was no use either...
> - GeoDataTreeModel has a 5times speed gain while using nodeType as const char* for type id rather than dynamic_cast !!!
> - A trick is done to connect the models only after all files have finished loading, another launch boost
> - the PlacemarkManager is removed: specific code to give population figures, roles and extra info is moved to FileLoader, and the remaining code was there to fill the MarblePlacemarkModel.
> 
> 
> Todo after this:
> - Continue removing MarblePlacemarkModel in Runners and Routing Model. 2 Options: 
>    - either a List of Placemarks is enough as a model, or
>    - a treemodel and proxies above a GeoDataDocument
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/FileLoader.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/FileViewWidget.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/GoToDialog.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarblePlacemarkModel.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleRunnerManager.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleRunnerManager.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleSearchListView.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleSearchListView.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/NavigationWidget.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/NavigationWidget.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkManager.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkManager.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkPainter.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/VisiblePlacemark.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataColorStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataColorStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataDocument.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataDocument.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataDocument_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFolder.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFolder.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataGeometry.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataGeometry.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataGeometry_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataHotSpot.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataHotSpot.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataIconStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataIconStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLabelStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLabelStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLatLonAltBox.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLatLonAltBox.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLatLonBox.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLatLonBox.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLinearRing.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLinearRing.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLinearRing_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLod.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLod.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLod_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLookAt.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPoint.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPoint.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPolyStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPolyStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPolygon.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPolygon.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPolygon_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataRegion.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataRegion.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataRegion_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyle.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyle.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyleMap.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyleMap.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyleSelector.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataStyleSelector.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimePrimitive.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimePrimitive.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimePrimitive_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeSpan.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeSpan.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeSpan_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeStamp.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeStamp.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataTimeStamp_p.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/kbihash_p.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/kdescendantsproxymodel.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/kdescendantsproxymodel.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingInputWidget.h 1202435 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingInputWidget.cpp 1202435 
>   /trunk/KDE/kdeedu/marble/tests/TestGeoData.cpp 1202435 
> 
> Diff: http://svn.reviewboard.kde.org/r/6040/diff
> 
> 
> Testing
> -------
> 
> Everything should work as usual.
> 
> Checked the following:
> - the NavigationWidget list of placemark is OK (should display updated list of placemarks as the treemodel is populated)
> - the PlacemarkLayout also displays placemarks from the new proxies stack
> - Search and Routing Runners still work
> - Routing model, routing points... work
> - Placemark Dialog (left click on a placemark) still display placemark properties
> - profiled and benchmarked for loading
> - (not checked) memory usage should have decreased
> 
> 
> Thanks,
> 
> Thibaut
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20101211/4edd8370/attachment-0001.htm

More information about the Marble-devel mailing list