[Marble-devel] Review Request: GpsArrow plugin replaces PositionTracking drawing of the gps position

[Torsten Rahn]

> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.h, line 75
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28806#file28806line75>
> >
> >     What is a "currentDraw"? Sounds like lottery to me. Maybe we should call this 
> >     "currentArrow" (if it does what I guess it does)?
> 
> Thibaut Gridel wrote:
>     The variables are those of PositionTracking... renaming m_arrow and m_previousArrow

Ah, sorry hadn't looked at that code for long. Well now would be a good opportunity to clean up names and comments :-)


> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp, line 119
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28807#file28807line119>
> >
> >     Can magnitude be zero? -> Booom!
> 
> Thibaut Gridel wrote:
>     Guess not, because of previous test ;)

Right. I had missed it.


> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp, line 153
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28807#file28807line153>
> >
> >     Why wouldn't you do this inside "update()"?
> >     Also I think that the whole m_previousDraw is redundant. You can determine the previousRegion as a local variable inside update() before you "overwrite" the m_currentDraw with the new shape. And then add the region of m_currentDraw to it after m_currentDraw has been modified.
> 
> Thibaut Gridel wrote:
>     I fear this wouldn't work to do the bookkeeping in update() because the update() / render() ratio is not 1 necessarily (property of the deferred painting timer), and we want to keep the last polygon that rendered, not the last one we calculated...

Hm, do we really want to keep the last polygon or just the last bounding box / region?


- Torsten


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


On 2010-06-15 21:58:55, Thibaut Gridel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4336/
> -----------------------------------------------------------
> 
> (Updated 2010-06-15 21:58:55)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> A new RenderPlugin to draw the gps position arrow instead of doing it in a specific way in PositionTracking and MarbleModel::paintGlobe.
> 
> Gps Track is deferred to GeometryModel, as the gps/ classes should go.
> 
> No more timer to cause updateGps, the position signal will trigger a repaintNeeded signal iff the position is visible
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/MarbleTest.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/Projections/AbstractProjection.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/Projections/AbstractProjection.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h 1136081 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp 1136081 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1136081 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/gps/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4336/diff
> 
> 
> Testing
> -------
> 
> - using/unusing the plugin, 
> - stopping/enabling gps tracks,
> - panning arrow in/out of view,
> - moving the map repaints the arrow at the new screen coordinates
> 
> The MarbleModel::showGps is losing some relevance here, as the plugin use/unuse replaces it somehow...
> 
> 
> Thanks,
> 
> Thibaut
> 
>