[Marble-devel] Review Request 118766: Customization option for drawn polygons

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118766/#review60127
-----------------------------------------------------------


Nice, clean code, thanks. Doing reviews with smaller chunks of code is much appreciated.


src/plugins/render/annotate/AnnotatePlugin.cpp
<https://git.reviewboard.kde.org/r/118766/#comment41880>

    tr( "Properties" ) instead?



src/plugins/render/annotate/EditPolygonDialog.h
<https://git.reviewboard.kde.org/r/118766/#comment41881>

    const QColor &color



src/plugins/render/annotate/EditPolygonDialog.h
<https://git.reviewboard.kde.org/r/118766/#comment41882>

    Private * const d



src/plugins/render/annotate/EditPolygonDialog.cpp
<https://git.reviewboard.kde.org/r/118766/#comment41885>

    the range here is 1..101 and does not match the one of the slider. Same below. Any chance you are trying to compensate for the implicit floor() of the integer arithmetic?
    
    E.g. a value of 5 in the range of 0..255 should be mapped to 2 in the range 0..100
    
    5 * 100 / 255 = 500 / 255 (=1.96) = 1
    round(5 * 100.0 / 255.0) = round(1.96) = 2
    
    I'd go for a simple
    qRound( lineStyle.color().alpha() / 2.55 ) );
    



src/plugins/render/annotate/EditPolygonDialog.cpp
<https://git.reviewboard.kde.org/r/118766/#comment41886>

    Same integer arithmetic problem as above, I'd use qRound( d->m_linesOpacity->value() * 2.55 ) instead. An even simpler approach would be to have the slider run from 0..255 directly.



src/plugins/render/annotate/EditPolygonDialog.cpp
<https://git.reviewboard.kde.org/r/118766/#comment41884>

    Using sender() is discouraged as it violates object orientation principles. In this case the code will even become shorter when you introduce two slots, updateLinesDialog and updatePolyDialog.


- Dennis Nienhüser


On June 15, 2014, 3:08 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118766/
> -----------------------------------------------------------
> 
> (Updated June 15, 2014, 3:08 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> I added a pretty simple customization option for drawn polygons which includes the following:
> - polygon area/lines color and opacity modifying;
> - lines width modifying;
> - polygon area filling can be set to "Not Filled".
> 
> Waiting for suggestions on what else would be nice to have as well as remarks about this implementation.
> 
> PS: It is what I have been working on for the last two days, so there is not too much code, but I think it is better than submitting large patches as I did last time.
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/annotate/AnnotatePlugin.h a401714 
>   src/plugins/render/annotate/AnnotatePlugin.cpp 2aca70e 
>   src/plugins/render/annotate/AreaAnnotation.cpp 1a4452b 
>   src/plugins/render/annotate/CMakeLists.txt 6caa034 
>   src/plugins/render/annotate/EditGroundOverlayDialog.ui 9873e8c 
>   src/plugins/render/annotate/EditPolygonDialog.h PRE-CREATION 
>   src/plugins/render/annotate/EditPolygonDialog.cpp PRE-CREATION 
>   src/plugins/render/annotate/EditPolygonDialog.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118766/diff/
> 
> 
> Testing
> -------
> 
> It works very nice.
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20140615/281acd03/attachment.html>