[Marble-devel] Re: Review Request: Position Marker trail

[Thibaut Gridel]

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


Very Good work, this patch makes trail working already!
I commented some nitpicks to make the code even easier to read/understand/maintain.


/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.h
<http://svn.reviewboard.kde.org/r/6313/#comment10620>

    We try to avoid arrays like this. This looks quite C-ish.
    The easy solution is to use a QList or a QVector instead of a table, and to define m_numTrailPoints as const.



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10621>

    The loop goes backwards, to set increasing size for bigger indexes of the array.
    Is there not a simpler way to do the same?
    (It's rather unusual to iterate tables backwards, unless there is a very good reason to. I think the code needs to present the list of past points as an increasing list.)
    Also comments to indicate the intent would be welcome.



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10622>

    Why not add <= in previous loop and remove this line?



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10623>

    Is there a specific interest to keep m_lastTrailPoint outside of this m_trail array?



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10624>

    if m_visibleTrailPoints is garanteed to be < m_numTrailPoints, could this loop be simplified, only using one counter?



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10625>

    Looks like m_trail should be an array of GeoDataCoordinates then, or better as said above, a QList or QVector.



/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp
<http://svn.reviewboard.kde.org/r/6313/#comment10626>

    sounds like you would want to use the painter state save and restore, to be sure it remains unaffected for next paintings?


- Thibaut


On Jan. 9, 2011, 9:01 p.m., Daniel Marth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6313/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2011, 9:01 p.m.)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Adds a trail to the position marker.
> GCI-task: http://www.google-melange.com/gci/task/show/google/gci2010/kde/t129364719879
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarkerConfigWidget.ui 1213205 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.h 1213205 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp 1213205 
> 
> Diff: http://svn.reviewboard.kde.org/r/6313/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Daniel
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20110114/4fa60094/attachment.htm