[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/#review10019
-----------------------------------------------------------


Looks good, need to figure where the Online Services plugins lose track of the Model or get lost on the way... the GeometryLayer works fine so the problem doesn't seem in the layermanager.
Those are mostly cosmetics comments.


/trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.cpp
<http://svn.reviewboard.kde.org/r/6624/#comment11217>

    could you rename the variable also?



/trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp
<http://svn.reviewboard.kde.org/r/6624/#comment11219>

    Could you rename?



/trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp
<http://svn.reviewboard.kde.org/r/6624/#comment11218>

    (Maybe OT) m_pluginManager is only used below currently. Is it expected?



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://svn.reviewboard.kde.org/r/6624/#comment11221>

    This one was a bit of a hack... 
    Putting it in such a visible place as MarbleModel public will be a reason to find a more elegant way to handle model updates during file loading.
    
    Can't we have friend for this one? (user is FileManager only)



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://svn.reviewboard.kde.org/r/6624/#comment11222>

    Need to drop the header and class MarbleDatafacade too ;)



/trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.h
<http://svn.reviewboard.kde.org/r/6624/#comment11223>

    Could you rename?



/trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.cpp
<http://svn.reviewboard.kde.org/r/6624/#comment11228>

    As we discussed, could you add a short comment here to remind that const prevents plugins from changing the model by design


- Thibaut


On March 22, 2011, 10:42 p.m., Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6624/
> -----------------------------------------------------------
> 
> (Updated March 22, 2011, 10:42 p.m.)
> 
> 
> Review request for marble.
> 
> 
> 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 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/BookmarkManagerDialog.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/FileManager.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/FileViewWidget.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/LayerManager.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/earthquake/EarthquakeModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/earthquake/EarthquakeModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/fileview/FileViewFloatItem.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/graticule/GraticulePlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/inhibit-screensaver/InhibitScreensaverPlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/opendesktop/OpenDesktopModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/opendesktop/OpenDesktopModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/overviewmap/OverviewMap.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/photo/PhotoPluginModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/photo/PhotoPluginModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/AbstractWeatherService.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCItemGetter.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCItemGetter.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCWeatherService.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/BBCWeatherService.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/FakeWeatherService.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/FakeWeatherService.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/WeatherModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/weather/WeatherModel.cpp 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/wikipedia/WikipediaModel.h 1225659 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/wikipedia/WikipediaModel.cpp 1225659 
> 
> 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/20110325/892646e2/attachment-0001.htm