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

[Bernhard Beschow]

> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp, line 67
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45743#file45743line67>
> >
> >     (Maybe OT) m_pluginManager is only used below currently. Is it expected?

Yeah, this one is not directly related to this patch.


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.cpp, line 43
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45749#file45749line43>
> >
> >     As we discussed, could you add a short comment here to remind that const prevents plugins from changing the model by design

I missed that one in the second revision of the patch. Done on my local machine, though.


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 240
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45745#file45745line240>
> >
> >     Need to drop the header and class MarbleDatafacade too ;)

MarbleDataFacade.* are dropped, but do not show up in the diff due to the way I prepared it.


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 127
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45745#file45745line127>
> >
> >     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)

done


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/RenderPlugin.h, line 63
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45748#file45748line63>
> >
> >     Could you rename?

done


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp, line 66
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45743#file45743line66>
> >
> >     Could you rename?

done


> On March 25, 2011, 10:17 p.m., Thibaut Gridel wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/AbstractDataPluginModel.cpp, line 93
> > <http://svn.reviewboard.kde.org/r/6624/diff/1/?file=45736#file45736line93>
> >
> >     could you rename the variable also?

done


- Bernhard


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


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/20110328/f7fa5d97/attachment-0001.htm