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

[Wes Hardaker]

> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h, line 8
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24947#file24947line8>
> >
> >     I don't think that this should be indented.

Most editors auto-indent stuff inside {} backets imposed by the namespace.  Personally, I prefer it indented because of that as well (since the whole point of indentation is to visually flag where nesting has happened).  I've removed it though.


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 52
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line52>
> >
> >     This looks strange. This won't be executed at all.

Yep; deleted (artifact)


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h, line 31
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24950#file24950line31>
> >
> >     This constructor has IMHO to many parameters. You can probably do this by setters, too. At least I hope this is possible ;).

The reason is that they're almost all required.  I can change the last two to setters, but the others would require a lot of error checking (in hard places to throw an error to the user) if they were unset.


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h, line 38
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24950#file24950line38>
> >
> >     Same here.

I've changed them to remove the last two.  Mind you these are cases where those variables should generally always be set.  It's actually an error to have an un-set seen from, though the code handles it ok.  In theory, the dump isn't needed but we're always setting it from a checkbox value anyway.


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h, line 13
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24957#file24957line13>
> >
> >     this should be openSocket()

fixed


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h, line 16
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24957#file24957line16>
> >
> >     this should be socket()
> >

fixed


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h, line 17
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24957#file24957line17>
> >
> >     and this setSocket()

fixed


> On 2010-04-29 06:34:01, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 121
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24956#file24956line121>
> >
> >     Where do these get deleted?
> >     I'd prefer doing:
> >     quit();
> >     if( wait(5000) ) {
> >         delete ;
> >     }
> >     else {
> >         mDebug() << "Thread bla couldn't have been stopped";
> >     }
> >     
> >     here.
> >     -

I'll work on cleaning those up.  I knew there was an issue somewhere that I forgot about, and it was the thread clean up.


- Wes


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


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