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

[Bastian Holst]

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


Cool plugin, thanks. I tested an old version, but I didn't have the time to test it now.
What I have done is a rough skimming over the source code.

There are some CODING-style problems in this source code. For example we use "a space inside both opening and closing parenthesis". Again have a look at http://websvn.kde.org/*checkout*/trunk/KDE/kdeedu/marble/CODING?revision=1112108.

I'll have another look at this patch later. I'll do some testing, too.


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

    I don't think that this should be indented.



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

    This looks strange. This won't be executed at all.



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

    This constructor has IMHO to many parameters. You can probably do this by setters, too. At least I hope this is possible ;).



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

    Same here.



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

    Where do these get deleted?
    I'd prefer doing:
    quit();
    if( wait(5000) ) {
        delete ;
    }
    else {
        mDebug() << "Thread bla couldn't have been stopped";
    }
    
    here.
    - 



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

    this should be openSocket()



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

    this should be socket()
    



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

    and this setSocket()


- Bastian


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