[Marble-devel] Review Request: GpsArrow plugin replaces PositionTracking drawing of the gps position
Torsten Rahn
rahn at kde.org
Wed Jun 16 15:54:33 CEST 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4336/#review6141
-----------------------------------------------------------
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.h
<http://reviewboard.kde.org/r/4336/#comment5749>
no compass here ;-)
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.h
<http://reviewboard.kde.org/r/4336/#comment5748>
const GeoDataCoordinates & position
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.h
<http://reviewboard.kde.org/r/4336/#comment5742>
What is a "currentDraw"? Sounds like lottery to me. Maybe we should call this
"currentArrow" (if it does what I guess it does)?
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5745>
These are local variables. So they shouldn't be prefixed m_* .
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5738>
Could you create a comment what this stuff is doing?
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5744>
Can magnitude be zero? -> Booom!
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5740>
Magic numbers
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5741>
Magic numbers
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5739>
Magic numbers
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5747>
It's good style to check whether the renderPos == HOVER_ABOVE_SURFACE at this point. If this is not the case the method should return.
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5746>
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.
/trunk/KDE/kdeedu/marble/src/plugins/render/gps/GpsArrow.cpp
<http://reviewboard.kde.org/r/4336/#comment5743>
please use a reference for parameters: const GeoDataCoordinates & position
- Torsten
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
>
>
More information about the Marble-devel
mailing list