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

Fri Nov 26 14:33:59 CET 2010

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > Some of my comments may address code that was just moved, not added.
> > Do you plan to commit that to 4.6 or 4.7?
> > I haven't tested the code yet, will do that tomorrow.

Thanks for your review.

Yes, I'd like to commit this patch for 4.6 already. It allows for some API cleanup which I tried to do already, but I had to revert some stuff since it didn't work out. See r1185076 and r1186008 for details.

I'd like to commit this patch ASAP since I'd like it to be tested by users of the beta2 release. Moreover, I'd like to give users of libmarblewidget enough time to adjust their own code. This involves just moving some method calls from MarbleModel objects to MarbleWidget/MarbleMap objects, but it still has to be done and communicated.

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp, line 379
> > <http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line379>
> >
> >     What's the reason to use the timer here? Any threads involved?

Again, I just moved the code. I think the intention here is to issue an update when all update activity is completed; that is, when the event loop is entered. I think the update mechanism could be implemented differently such that timer is not necessary. Like above, this should be done in a different patch.

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp, line 87
> > <http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line87>
> >
> >     Is it safe with that little error checking? index may be invalid, internal pointer null, dynamic_cast may return 0. I'd prefer some Q_ASSERTs at least.

Your comment is probably very true. I think this should be solved in a different patch since I'd like to solve only one issue (separation of concernes WRT model-view) with this patch. Ideally, this patch should neither introduce nor remove any bugs.

IIUC, idis is working on a rewrite of the tree model anyway.

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp, line 208
> > <http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line208>
> >
> >     Must be initialized. Not all code paths of propertyValue() ensure that the value is set, so you may end up using them unitialized later.
> >

Agreed.

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h, line 1086
> > <http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41482#file41482line1086>
> >
> >     Foo const * const() or Foo * () as above

Agreed. I'll fix this old (moved) code.

> On 2010-11-24 20:43:36, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h, line 848
> > <http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41477#file41477line848>
> >
> >     Preferably returning a pointer to a const object -- or the method shouldn't be const

Agreed.

- Bernhard

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

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/20101126/9cecde85/attachment.htm 