[Marble-devel] Re: Review Request: Remove MarbleDataFacade; use MarbleModel instead

[Thibaut Gridel]

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

Ship it!


I'd say shipit, as it simplifies code and brings the model data through a real const for plugins.
Good work!

- Thibaut


On March 28, 2011, 8:19 p.m., Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6624/
> -----------------------------------------------------------
> 
> (Updated March 28, 2011, 8:19 p.m.)
> 
> 
> Review request for marble and Thibaut Gridel.
> 
> 
> Summary
> -------
> 
> MarbleDataFacade is basically a wrapper for MarbleModel and is used in RenderPlugins. Since MarbleModel is exported, MarbleDataFacace seems redundant.
> 
> Besides porting RenderPlugins to use const MarbleModel* rather than MarbleDataFacade*, the patch introduces the following changes:
> * move all models, trees, and proxies from MarbleDataFacade to MarbleModel
> * introduce convenience method MarbleModel::planetId() const;
> 
> Please have a look if there lifetime issues.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/BookmarkManagerDialog.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/FileViewWidget.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/LayerManager.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/earthquake/EarthquakeModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/earthquake/EarthquakeModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/fileview/FileViewFloatItem.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/inhibit-screensaver/InhibitScreensaverPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/opendesktop/OpenDesktopModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/opendesktop/OpenDesktopModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/overviewmap/OverviewMap.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/photo/PhotoPluginModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/photo/PhotoPluginModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/AbstractWeatherService.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCItemGetter.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCItemGetter.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCWeatherService.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCWeatherService.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/FakeWeatherService.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/FakeWeatherService.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/WeatherModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/WeatherModel.cpp 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/wikipedia/WikipediaModel.h 1226333 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/wikipedia/WikipediaModel.cpp 1226333 
> 
> Diff: http://svn.reviewboard.kde.org/r/6624/diff
> 
> 
> Testing
> -------
> 
> there are issues with plugins that appear before and after the patch is applied
> * the Wikipedia plugin doesn't show items upon startup
> * the solcialdesktop plugin leads to a crash when closing Marble
> 
> 
> Thanks,
> 
> Bernhard
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20110329/f3599690/attachment.htm