[Marble-devel] Re: Review Request: Move painting methods from MarbleModel to MarbleMap

[Thibaut Gridel]

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

Ship it!


A first look at the patch, excellent work. The hidden comment "This function will probably move to MarbleWidget in KDE4.1, making the MarbleModel/MarbleWidget pair truly follow the Model/View paradigm" is long overdue!


/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://svn.reviewboard.kde.org/r/5902/#comment9649>

    model seems not used anymore here



/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp
<http://svn.reviewboard.kde.org/r/5902/#comment9652>

    In a later version, could be useful to have layers react directly to theme change.
    For instance, veccomposer would update its properties in its own slot for setMapThemeIt



/trunk/KDE/kdeedu/marble/src/lib/MarbleMap_p.h
<http://svn.reviewboard.kde.org/r/5902/#comment9650>

    I guess most of this code was moved, so more changes could still wait a bit.
    
    Is it expected to change to pointers here, at least for all layers so they could simply be added to the LayerManager, and have a consistent manipulation?



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp
<http://svn.reviewboard.kde.org/r/5902/#comment9651>

    Don't know enough of the StackedTileLoader to determine if it is more a model store or a layer rendering class...
    
    Maybe this class could benefit from a split, with storing in model and rendering in map? (maybe it doesn't have sense to split so...)
    
    Is it used in GL with either role?


- Thibaut


On 2010-11-19 16:07:35, Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5902/
> -----------------------------------------------------------
> 
> (Updated 2010-11-19 16:07:35)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Motivation
> ==========
> 
> The whole painting should be implemented in the view rather than in the model because it decouples data and its representation. This allows for having different graphics backends (hello OpenGL!) while having a common interface to the data.
> 
> 
> Implementation
> ==============
> 
> MarbleMap is now the owner of all painting objects, e.g. the MeasureTool, a new TextureLayer class, the LayerManager, and others. To see all painting objects owned by MarbleMap, see the modified MarbleMap_p.h.
> 
> Since the LayerManager object is now owned by MarbleMap, the layer-relevant methods, slots, and signals (such as addLayer(), removeLayer(), renderPluginsInitialized()) are now forwarded by MarbleMap and Marblewidget rather than by MarbleModel.
> 
> The DownloadRegionDialog has been modified to take a MarbleWidget instead of a MarbleModel. I wonder if it makes more sense to take a MarbleMap here.
> 
> Please test if the patch works for you as well. Since the patch changes some initializaton orders in the constructors and some re-initialization orders on theme change, it is important to test these code paths more extensively.
> 
> If you see that the patch violates some assumptions/invariants, please report as well.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/ControlView.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap_p.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetPopupMenu.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.h 1198738 
>   /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/marble_part.cpp 1198738 
>   /trunk/KDE/kdeedu/marble/src/plasmoid/worldclock.cpp 1198738 
> 
> Diff: http://svn.reviewboard.kde.org/r/5902/diff
> 
> 
> Testing
> -------
> 
> Works for me using the KDE version in the majority of cases.
> 
> 
> Thanks,
> 
> Bernhard
> 
>

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