[Marble-devel] Review Request: add APRS data display to marble

Thu May 6 00:06:44 CEST 2010

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

If you ask me, your patch is near “Ship It”. There are some problems, mostly minor, that could need a correction.
One thing is that the the indentation of your headers isn't Marble style, is it?

On the other hand I'm not sure if we can include “Public Domain” code in Marble. Hm ...

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h
<http://reviewboard.kde.org/r/3843/#comment5121>

    What does this do?
    Either you should change the name here or you should add a comment.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h
<http://reviewboard.kde.org/r/3843/#comment5122>

    Could you please try to explain what direct means in the source code as a comment.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h
<http://reviewboard.kde.org/r/3843/#comment5123>

    perhaps m_errorCount would be a better name here as it does not contain abbreviations

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment5124>

    A short comment would make this clearer, too.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment5125>

    Sorry for being a bit pedantic here, but there is a missing space ;)

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment5126>

    Perhaps you can add a "aprs: " as a prefix for your mDebug() messages

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp
<http://reviewboard.kde.org/r/3843/#comment5127>

    We make a newline after every '}'

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp
<http://reviewboard.kde.org/r/3843/#comment5128>

    Again, no newline after '}' and missing spaces.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5129>

    Right, this seems to be missing here and seems to be a memory leak.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5130>

    If wait isn't sucessful, this will crash here.
    Same in the case ( m_tcpipGatherer == 0 ).
    You should only delete them if ( m_tcpipGatherer == 0 ) and wait was successful.

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5131>

    Missing space

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5132>

    missing space

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5133>

    missing space

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp
<http://reviewboard.kde.org/r/3843/#comment5134>

    missing space

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.cpp
<http://reviewboard.kde.org/r/3843/#comment5135>

    missing space

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.cpp
<http://reviewboard.kde.org/r/3843/#comment5136>

    a line

trunk/KDE/kdeedu/marble/src/plugins/render/aprs/posix_qextserialport.cpp
<http://reviewboard.kde.org/r/3843/#comment5137>

    Is this an allowed license. Are we allowed to relicense it to LGPL?

    In any case, we need a clear license header.

- Bastian

On 2010-05-04 02:54:40, Wes Hardaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3843/
> -----------------------------------------------------------
> 
> (Updated 2010-05-04 02:54:40)
> 
> 
> 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/src/plugins/render/aprs/AprsFile.cpp PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h PRE-CREATION 
>   trunk/KDE/kdeedu/marble/data/CMakeLists.txt 1121250 
>   trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1121250 
>   trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsConfigWidget.ui 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
> 
>