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

Wes Hardaker wjhns25 at hardakers.net
Tue May 11 06:44:09 CEST 2010 


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h, line 26
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25553#file25553line26>
> >
> >     What does this do?
> >     Either you should change the name here or you should add a comment.

comment added.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h, line 29
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25553#file25553line29>
> >
> >     Could you please try to explain what direct means in the source code as a comment.

comment added.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.h, line 33
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25553#file25553line33>
> >
> >     perhaps m_errorCount would be a better name here as it does not contain abbreviations

Changed.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 71
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25557#file25557line71>
> >
> >     A short comment would make this clearer, too.

added.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 75
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25557#file25557line75>
> >
> >     Sorry for being a bit pedantic here, but there is a missing space ;)

yep.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 77
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25557#file25557line77>
> >
> >     Perhaps you can add a "aprs: " as a prefix for your mDebug() messages

done


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 144
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25557#file25557line144>
> >
> >     We make a newline after every '}'

done


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 98
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25560#file25560line98>
> >
> >     Again, no newline after '}' and missing spaces.

done


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 221
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line221>
> >
> >     Missing space

done


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 259
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line259>
> >
> >     missing space

added.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 276
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line276>
> >
> >     missing space

added


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 284
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line284>
> >
> >     missing space

added


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.cpp, line 75
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25566#file25566line75>
> >
> >     missing space

added


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.cpp, line 32
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25571#file25571line32>
> >
> >     a line

nuked.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/posix_qextserialport.cpp, line 5
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25572#file25572line5>
> >
> >     Is this an allowed license. Are we allowed to relicense it to LGPL?
> >     
> >     In any case, we need a clear license header.

Public domain generally means "unclaimed" which means anyone can re-license it.  I think.  But I'm not a lawyer.


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 125
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line125>
> >
> >     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.

fixed


> On 2010-05-05 22:06:54, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 59
> > <http://reviewboard.kde.org/r/3843/diff/5/?file=25562#file25562line59>
> >
> >     Right, this seems to be missing here and seems to be a memory leak.

Properly cleaned now


- Wes


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


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

More information about the Marble-devel mailing list