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

Wes Hardaker wjhns25 at hardakers.net
Fri Apr 30 05:11:21 CEST 2010 


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.h, line 28
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24953#file24953line28>
> >
> >     Different to addSeenFrom?

Yes.  It's a bit vector.  setSeenFrom will set the full set of bits, addSeenFrom adds in new ones (when seen from new sources that need to be merged with old).


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 22
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line22>
> >
> >     Move to the bottom of the file, if possible

deleted actually.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 27
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line27>
> >
> >     Intentionally initializing the QString with 0 here? Looks odd.

good point; fixed.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 35
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line35>
> >
> >     Intentionally initializing the QString with 0 here? Looks odd.

good point; fixed.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 93
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line93>
> >
> >     The method name indicates a setter, but it is more (calculates a suitable color, sets it and returns it). I'd find it easier to understand if the method was 
> >     QColor calculatePaintColor(int from, QTime time, int fadetime)
> >     and the caller would be responsible to modify the painter.
> >

good point; changed.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsObject.cpp, line 94
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24954#file24954line94>
> >
> >     Method can be const (another reason not to prefix it with set)

CHanged it to const; good idea.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 39
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24956#file24956line39>
> >
> >     use an unnamed namespace to avoid naming conflicts.
> >     namespace {
> >     const int timeBetweenDownloads = 30000;
> >     }

Actually, that was a carry-over and can be (was now) deleted.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 350
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24956#file24956line350>
> >
> >     What's that used for?

Err...  I was trying to get started on actions, but not having done them before I threw in some random code.  Most of that should be deleted before submission (and now has been).


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 42
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24956#file24956line42>
> >
> >     Leaking m_mutex

Yeah, I need cleanup on delete; it's tricky with threads but fortunately the plugin shouldn't be recreated much.  I'll fix this eventually.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h, line 20
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24957#file24957line20>
> >
> >     const?

Yep. done.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsSource.h, line 14
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24957#file24957line14>
> >
> >     const?

Yep.  done.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsPlugin.cpp, line 366
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24956#file24956line366>
> >
> >     See above. Debug leftovers?

deleted


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.cpp, line 38
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24960#file24960line38>
> >
> >     Local variables should not be prefixed m_

fixed


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/AprsTCPIP.cpp, line 42
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24960#file24960line42>
> >
> >     Hardcoded hostname / port vs. members?

whoops; fixed.


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.h, line 28
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24964#file24964line28>
> >
> >     const?

made const


> On 2010-04-29 20:09:21, Dennis Nienhüser wrote:
> > trunk/KDE/kdeedu/marble/src/plugins/render/aprs/GeoAprsCoordinates.h, line 30
> > <http://reviewboard.kde.org/r/3843/diff/1/?file=24964#file24964line30>
> >
> >     const?

made const


- Wes


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


On 2010-04-29 21:08:12, Wes Hardaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3843/
> -----------------------------------------------------------
> 
> (Updated 2010-04-29 21:08:12)
> 
> 
> 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
> 
>

More information about the Marble-devel mailing list