-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3843/#review5292
-----------------------------------------------------------
Just read over the first part yet, will finish it in the evening.
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp
<http://reviewboard.kde.org/r/3843/#comment4781>
Initialize m_numErrors to zero here
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp
<http://reviewboard.kde.org/r/3843/#comment4783>
qDebug (and mDebug) append newlines automatically
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp
<http://reviewboard.kde.org/r/3843/#comment4788>
Something is wrong here. Two return statements in sequence.
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp
<http://reviewboard.kde.org/r/3843/#comment4789>
Looks dangerous to me as the provided socket argument becomes a dangling pointer depending on the state. Fine for the socket = check_read_return(len, socket) usage, but maybe change the method signature to
void AprsFile::check_read_return(int length, QIODevice **socket)
to make the intended usage clear (and use check_read_return(len, &socket) in the code then
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h
<http://reviewboard.kde.org/r/3843/#comment4791>
Preferably GeoDataCoordinates instead of float lat, float lon arguments
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment4793>
Better use float(m_dstcallDigits[dstcall[1]]) instead of (float) m_dstcallDigits[dstcall[1]], i.e. ctor-like conversion instead of c-style casts. Alternatively, *10.0 instead of *10.
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment4794>
Initialize to zero (helps to detect refactoring errors when accessing the object too early)
trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment4795>
Uh oh... a dragon ;-)
It depends on the inclusion order?
- Dennis
On 2010-04-29 05:30:51, Wes Hardaker wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3843/
> -----------------------------------------------------------
>
> (Updated 2010-04-29 05:30:51)
>
>
> Review request for marble.
>
>
> Summary
> -------
>
> Adds support for displaying HAM radio aprs data to the map pulled from three different possible sources. The TCP/IP source is the default that is enabled and usable by everyone. The others would require special sources and default to off. The icons, which are binary files, can be found at: http://www.hardakers.net/code/marble/aprs-icons.tar.gz
>
>
> Diffs
> -----
>
> trunk/KDE/kdeedu/marble/data/CMakeLists.txt 1089346
> trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1089346
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsConfigWidget.ui PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherGen.pl PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer_mic_e.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTTY.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTTY.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/CMakeLists.txt PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/posix_qextserialport.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/qextserialport.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/qextserialport.cpp PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/qextserialport_global.h PRE-CREATION
> trunk/KDE/kdeedu/marble/src/plugins/render/aprs/win_qextserialport.cpp PRE-CREATION
>
> Diff: http://reviewboard.kde.org/r/3843/diff
>
>
> Testing
> -------
>
>
> Screenshots
> -----------
>
> screenshot of california with data
> http://reviewboard.kde.org/r/3843/s/378/
>
>
> Thanks,
>
> Wes
>
>