[Marble-devel] Review Request 109379: ElevationProfile: support for gpx-tracks

[Dennis Nienhüser]

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


Thanks, lovely patch :-)

Some nitpicking below. I'd prefer if the individual data sources had their own file for each class.


src/plugins/render/elevationprofilefloatitem/ElevationProfileContextMenu.h
<http://git.reviewboard.kde.org/r/109379/#comment23584>

    ctor should be marked explicit



src/plugins/render/elevationprofilefloatitem/ElevationProfileContextMenu.h
<http://git.reviewboard.kde.org/r/109379/#comment23576>

    QMenu* menu() as per Qt API style



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23579>

    Can the second parameter be a const reference as well?



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23583>

    ctor should be marked explicit



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23580>

    getElevation => elevation
    the parameter should be a const reference



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23581>

    should either not be const or return a const model



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23582>

    ctor should be marked explicit



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h
<http://git.reviewboard.kde.org/r/109379/#comment23585>

    ctor should be marked explicit
    marbleMdl => marbleModel



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23587>

    ++i



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23589>

    Ideally also save placemark->name() here which can be used as a nicer user visible string than 'filename: 1' ... as calculated below.
    



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23588>

    should be i+1 since it is a user visible string and those shouldn't be 0 based.
    
    Ideally this would only be the fallback if the placemark containing the track doesn't have a name (see comment above).



src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23590>

    ++i



src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23591>

    yes



src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23592>

    I'd shorten "load a track from file" to "open a track"



src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.cpp
<http://git.reviewboard.kde.org/r/109379/#comment23593>

    please remove both comments


- Dennis Nienhüser


On April 16, 2013, 5:19 p.m., Roman Karlstetter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109379/
> -----------------------------------------------------------
> 
> (Updated April 16, 2013, 5:19 p.m.)
> 
> 
> Review request for Marble and Bernhard Beschow.
> 
> 
> Description
> -------
> 
> Changes to the elevation profile plugin, it's now possible to display the height profile for a GPX-track opened from file.
> 
> I made a new classes for
> a) providing the data, one for routes and one for tracks. They are in the same file, should this be split into multiple files?
> b) the contextmenu, because it has some members which would only clutter ElevationProfileFloatItem. It is a friend class, is this ok?
> 
> Comments please :) 
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileContextMenu.h PRE-CREATION 
>   src/plugins/render/elevationprofilefloatitem/CMakeLists.txt 872e5e1 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileContextMenu.cpp PRE-CREATION 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.h PRE-CREATION 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileDataSource.cpp PRE-CREATION 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.h cb3bff1 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.cpp 7cb3428 
> 
> Diff: http://git.reviewboard.kde.org/r/109379/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Roman Karlstetter
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20130427/1ef2650a/attachment-0001.html>