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

[Torsten Rahn]

> On 2010-04-29 09:32:03, Torsten Rahn wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 26
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line26>
> >
> >     What is "Direct"? :-)
> 
> Wes Hardaker wrote:
>     Not heard through a repeater.  IE, they're close enough that your antenna can pick them up directly.

Well what I was hinting at was that maybe there is a more intuitive way to name this which would better get the point across. But that might not necessarily be the case. So it might be fine as is.


> On 2010-04-29 09:32:03, Torsten Rahn wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h, line 36
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24950#file24950line36>
> >
> >     Should be an enum.
> 
> Wes Hardaker wrote:
>     Why?  It's either supposed to dump or not.  That's very boolean to me.

Well, this is a plugin and not public API, so this issue is not that important. However in general it's a matter of good API design to only use bools where the boolean meaning is obvious:

joke->setEnabled(true); // the meaning of the boolean is obvious in this case

Humour * joke = new Humour( "Programmer's joke", true ); // the meaning of the boolean in this case is not obvious 

Humour * joke = new Humour( "Programmer's joke", RollingOnTheFloorLaughing ); // using an enum it's much more readable :-)


> On 2010-04-29 09:32:03, Torsten Rahn wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer_mic_e.h, line 347
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24952#file24952line347>
> >
> >     The icons don't really match our Quality Requirements. They should use Oxygen colors. Ideally Nuno Pinheiro would create some which would match the required style.
> 
> Wes Hardaker wrote:
>     The icons were taken from somewhere else; I'm not an artist and there is no way I could have done them!  I'd love to have someone do better versions that what is available today.
>     
>     I need to go chase down the license for the current one (I thought it was on the page I got them from as GPL-ish but I can't find it now).

Cool. Thanks :-)


> On 2010-04-29 09:32:03, Torsten Rahn wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 241
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24951#file24951line241>
> >
> >     threebytes.first().toAscii() ....
> 
> Wes Hardaker wrote:
>     AprsGatherer.cpp:235: error: 'const class QString' has no member named 'first'

Ooops. I assumed it was a container.


> On 2010-04-29 09:32:03, Torsten Rahn wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 98
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line98>
> >
> >     Please use Oxygen Colors.
> 
> Wes Hardaker wrote:
>     changed everywhere to stuff from the pallette

Cool thanks. :-)


- Torsten


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


On 2010-04-30 03:27:20, Wes Hardaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3843/
> -----------------------------------------------------------
> 
> (Updated 2010-04-30 03:27:20)
> 
> 
> 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 1119036 
>   trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1119036 
>   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
> 
>