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

Thu Apr 29 11:31:58 CEST 2010

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


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

    License Header (LGPL) missing


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

    AprsFile( const QString & ttyName )


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

    License Header (our policy would require it to be LGPL or BSD) missing


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

    const QString & filename


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

    What is "Direct"? :-)


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

    We always use preincrement when preincrement is meant. So ++m_numErrors.


trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherGen.pl
<http://reviewboard.kde.org/r/3843/#comment4803>

    License Header (our policy would require it to be LGPL or BSD) missing.


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

    Should be an enum.


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

    void addObject( const QString & callSign, ....


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

    routePath, symTable, symCode


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

    m_dumpOutput


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

    seenFrom


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

    sourceName


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

    Enum


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

    m_sourceName(),


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

    bool dumpOutput


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

    m_dumpOutput(dumpOutput),


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

    m_seenFrom(seenFrom),


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

    bool canDoDirect = .....


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

    lineLength ....


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

    lineLength


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

    m_dumpOutput


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

    What is XXX ?


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

    callSign, ... canDoDirect


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

    --wheeeee


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

    dstCallDigits


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

    myCall, canDoDirect


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

    const QString& const QChar &


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

    for coordinates it's recommendable to use qreal instead of "float".


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

    please use const references for QString and QChar as a method parameter! :)  const QString&, const QChar &


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

    newObject. But also you should avoid C++ keywords (like new) as part of member names.


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

    I like jokes, but I think we should keep them to a minimum in the code ;-)


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

    camel casing - unless there is a good reason (like copy paste code or C code).


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

    qreal AprsGatherer::calculateLongitude( const QString&, ... )


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

    isEast or rather use an enum.


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

    threebytes.first().toAscii() ....


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

    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.


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

    const QString &


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

    getters don't get prefixed with "get". Just setters are prefixed with "set".


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

    fadeTime, hideTime


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

    m_myName


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

    m_seenFrom


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

    const QString &


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

    please avoid "new" (and use camel casing)


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

    "setPixmap" should use a QPixmap as a parameter. If you want to use a QString for describing the pixmap I'd rather use setPixmapId( const QString & pixmap )


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

    Please use Oxygen Colors.


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

    Please use Oxygen Colors. 


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

    Please use Oxygen Colors. 


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

    Please use Oxygen Colors. 


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

    Please use Oxygen Colors. 


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

    Q_Assert?


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

    ++spot


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

    ++spot, ++lastSpot


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

    camelCasing


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

    camelCasing 


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

    camelCasing 


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

    camelCasing 


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

    camelCasing 


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

    camelCasing 


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

    camelCasing 


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

    // Plugin is disabled by default


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

    // Plugin is visibile by default


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

    This should really be a name that is commonly understood by any Marble user. I've never heard of "Aprs" but if it was named "Ham Radio" or "Amateur Radio" or something more known that would be better.


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

    See comment on line 89


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

    This should explain in more detail what APRS is about.


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

    What is XXX? Should this be "FIXME:" ?


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

    see above


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

    see above


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

    you should use a const iterator here.


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

    ++obj
    and
    constBegin(), constEnd()


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

    License !


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

    const QString &


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

    ++m_numErrors;


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

    License!


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

    const QString&


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

    m_ttyName


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

    ++m_numErrors;


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

    License!


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

    GeoDataCoordinates will probably made non-inheritable soonish.
    
    What you probably want to use here instead is a GeoDataPlacemark.
    
    But for now this is ok. 


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

    use qreal or GeoDataCoordinates where possible :-)


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

    setTimeStamp( const QTime& );
    

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

    timeStamp(); (no "get")


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

    What the license of this file? We need it to be LGPL or BSD style.


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

    LICENSE!


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

    License!


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

    License!


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

    License!


- Torsten


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