[Marble-devel] Review Request: Adds Measure Tool Plugin, with all of the functionality of the Measure tool.

[Dennis Nienhüser]

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


Great work! I didn't test it yet, but the code looks good. Some minor nitpicking below. Note that some comments address code which you just moved, feel free to skip those.


src/plugins/render/measure/MeasureToolPlugin.h
<http://git.reviewboard.kde.org/r/103262/#comment7223>

    Each on its own line, please.



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7224>

    Just measure-tool, the other Ids don't have a plugin at the end as well.



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7225>

    A bit too technical for the average user. I'd suggest "Measure distances between two or more points." instead
    



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7226>

    Use 0 instead of NULL. Preferably initialize it in the init list above like so:
    m_pen( Qt::red ), m_marbleWidget( 0 )



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7227>

    Please replace this if construct and the boolean variable above and the setRenderHint below with a simple call to
    painter->autoMapQuality();
    which does the same.
    



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7228>

    A fixed size array looks so C-ish :-/
    I know it's not your code, but maybe we should refactor this one.
    



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7229>

    markRadius. Not your code again.



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7230>

    Spaces are inconsistent here (space missing after this), spaces missing for the booleans.



src/plugins/render/measure/MeasureToolPlugin.cpp
<http://git.reviewboard.kde.org/r/103262/#comment7231>

    0 instead of NULL.


- Dennis Nienhüser


On Nov. 26, 2011, 11:41 p.m., Mike Henning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103262/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2011, 11:41 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> Adds Measure Tool Plugin, with all of the functionality of the Measure tool.
> 
> This patch depends on https://git.reviewboard.kde.org/r/103261/
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/CMakeLists.txt de8e5e5 
>   src/plugins/render/measure/CMakeLists.txt PRE-CREATION 
>   src/plugins/render/measure/MeasureToolPlugin.h PRE-CREATION 
>   src/plugins/render/measure/MeasureToolPlugin.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103262/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mike Henning
> 
>

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