[Marble-devel] Review Request 123259: Added manual rotation to the GroundOverlayFrame in edit mode.

[Torsten Rahn]

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



src/plugins/render/annotate/GroundOverlayFrame.h (line 72)
<https://git.reviewboard.kde.org/r/123259/#comment53710>

    This naming got me irritated only now :-) Of course it was there before (with movedPoint*). But maybe we should rename the "Point" to "Handle" (hoveredPoint would then become e.g. hoveredHandle. Point otherwise makes me think of a QPoint (which it's not).



src/plugins/render/annotate/GroundOverlayFrame.h (line 73)
<https://git.reviewboard.kde.org/r/123259/#comment53709>

    I guess this should be m_editStatus to clearly convey what it's about.



src/plugins/render/annotate/GroundOverlayFrame.h (line 74)
<https://git.reviewboard.kde.org/r/123259/#comment53707>

    This variable name doesn't immediately convey to me what it's about. 
    I guess it's not about having focus, is it? :-)



src/plugins/render/annotate/GroundOverlayFrame.h (line 75)
<https://git.reviewboard.kde.org/r/123259/#comment53708>

    "last" is usually used as a prefix for a last object in a row (opposed to first). 
    In this case I'd rather use "previous" as a prefix.



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 43)
<https://git.reviewboard.kde.org/r/123259/#comment53700>

    Could you move the arrow icons into its own subdirectory /bitmaps/editarrows ? :-)



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 161)
<https://git.reviewboard.kde.org/r/123259/#comment53701>

    we prefer preincrements (++i) where possible.



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 163)
<https://git.reviewboard.kde.org/r/123259/#comment53706>

    This calculation doesn't work too well for the pure positioning of the handle icons: 
    - the edge icons appear off from the usually bent line
    - as soon as you drag the handles they sometimes start to jump
    
    We need something better here: Spherical Interpolation to the rescue! :-)
    
    - For the corners it would be appropriate to use the ring.at() positions
    - For the edges you could use
    
    GeoDataCoordinates interpolate( const GeoDataCoordinates &target, double t ) 
    
    i.e.
    
    ring.at().interpolate( ring.at(), 0.5 ) 
    
    This should yield better results.
    
    As a result we probably don't need the NorthernRegion, SouthernRegion, EasternRegion and WesternRegion code anymore, do we (if so, removing it might speed up things again I guess).



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 172)
<https://git.reviewboard.kde.org/r/123259/#comment53702>

    Please apply
    
    transformed( trans,  Qt::SmoothTransformation) )
    
    to avoid jagginess.



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 174)
<https://git.reviewboard.kde.org/r/123259/#comment53703>

    Please apply
    
    transformed( trans,  Qt::SmoothTransformation) )
    
    to avoid jagginess.



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 179)
<https://git.reviewboard.kde.org/r/123259/#comment53704>

    Please apply
    
    transformed( trans,  Qt::SmoothTransformation) )
    
    to avoid jagginess.



src/plugins/render/annotate/GroundOverlayFrame.cpp (line 183)
<https://git.reviewboard.kde.org/r/123259/#comment53705>

    Please apply
    
    transformed( trans,  Qt::SmoothTransformation) )
    
    to avoid jagginess.


- Torsten Rahn


On April 5, 2015, 5:03 vorm., Dávid Kolozsvári wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123259/
> -----------------------------------------------------------
> 
> (Updated April 5, 2015, 5:03 vorm.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Until now it wasn't possible to rotate the GroundOverlayFrame manually, only from it's properties menu, setting the angle in a spinbox.
> This patch introduces a user-friendly manual rotation in edit mode, as well as new icons(thanks to Torsten Rahn) which are shown as handles around it's edges and corners.
> Also, a bug was fixed in this patch: the GroundOverlayFrame wasn't able to capture the mouse events that occured outside of it's regions while a controlpoint(handle) was grabbed.
> 
> 
> Diffs
> -----
> 
>   data/bitmaps/arrow-diagonal-topleft-active.png PRE-CREATION 
>   data/bitmaps/arrow-diagonal-topleft.png PRE-CREATION 
>   data/bitmaps/arrow-diagonal-topright-active.png PRE-CREATION 
>   data/bitmaps/arrow-diagonal-topright.png PRE-CREATION 
>   data/bitmaps/arrow-horizontal-active.png PRE-CREATION 
>   data/bitmaps/arrow-horizontal.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-bottomleft-active.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-bottomleft.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-bottomright-active.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-bottomright.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-topleft-active.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-topleft.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-topright-active.png PRE-CREATION 
>   data/bitmaps/arrow-rotation-topright.png PRE-CREATION 
>   data/bitmaps/arrow-vertical-active.png PRE-CREATION 
>   data/bitmaps/arrow-vertical.png PRE-CREATION 
>   src/plugins/render/annotate/AnnotatePlugin.cpp 45e8746 
>   src/plugins/render/annotate/GroundOverlayFrame.h 194ade5 
>   src/plugins/render/annotate/GroundOverlayFrame.cpp c27eabd 
>   src/plugins/render/annotate/SceneGraphicsItem.h c59340e 
> 
> Diff: https://git.reviewboard.kde.org/r/123259/diff/
> 
> 
> Testing
> -------
> 
> Testing was done by me and it works fine.
> 
> 
> Thanks,
> 
> Dávid Kolozsvári
> 
>

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