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

Thu Apr 29 21:48:52 CEST 2010

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 11
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line11>
> >
> >     Initialize m_numErrors to zero here

done

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 42
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line42>
> >
> >     qDebug (and mDebug) append newlines automatically

Carry over from my using cerr to not require full debugging output...  Never removed the \ns.  Thanks for the reminder.

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 51
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line51>
> >
> >     Something is wrong here. Two return statements in sequence.

First deleted.

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsFile.cpp, line 52
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24948#file24948line52>
> >
> >     Looks dangerous to me as the provided socket argument becomes a dangling pointer depending on the state. Fine for the socket = check_read_return(len, socket) usage, but maybe change the method signature to
> >     void AprsFile::check_read_return(int length, QIODevice **socket)
> >     to make the intended usage clear (and use check_read_return(len, &socket) in the code then

Actually that delete shouldn't be there (if you looked above it there was a return).

However, your point is still potentially valid about the usage of the function in the other classes.  I've made the change you suggested everywhere.

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 234
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24951#file24951line234>
> >
> >     Uh oh... a dragon ;-)
> >     It depends on the inclusion order?

The include file is just to include a generated set of code (actually, some of it can be algorithmic and I should switch that back). Much of the mic-e format is very odd and requires a lookup-table.  That's all the mic_e header is really doing: defining the lookup-table contents.

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 157
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24951#file24951line157>
> >
> >     Better use float(m_dstcallDigits[dstcall[1]]) instead of (float) m_dstcallDigits[dstcall[1]], i.e. ctor-like conversion instead of c-style casts. Alternatively, *10.0 instead of *10.

done

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.cpp, line 195
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24951#file24951line195>
> >
> >     Initialize to zero (helps to detect refactoring errors when accessing the object too early)

done

> On 2010-04-29 06:23:47, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsGatherer.h, line 48
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24950#file24950line48>
> >
> >     Preferably GeoDataCoordinates instead of float lat, float lon arguments

changed to qreal but didn't change to a class yet; the issue is a bit more tricky than that.

- Wes

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

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