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

[Thibaut Gridel]

> 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)?

The variables are those of PositionTracking... renaming m_arrow and m_previousArrow


> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp, line 112
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28807#file28807line112>
> >
> >     These are local variables. So they shouldn't be prefixed m_* .

refactoring typo ;) used to be class members...


> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp, line 116
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28807#file28807line116>
> >
> >     Could you create a comment what this stuff is doing?

If only I knew... The code is that of PositionTracking... after some trivial simplifications
Will add some commenting though


> 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!

Guess not, because of previous test ;)


> 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.

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...


> On 2010-06-16 13:54:37, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp, line 157
> > <http://reviewboard.kde.org/r/4336/diff/1/?file=28807#file28807line157>
> >
> >     please use a reference for parameters: const GeoDataCoordinates & position

thanks!


- Thibaut


-----------------------------------------------------------
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
> 
>