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

Thibaut Gridel tgridel at free.fr
Sat Dec 4 01:14:25 CET 2010 

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

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/20101204/6e23b0c1/attachment-0001.htm

More information about the Marble-devel mailing list