[Marble-devel] Review Request 118405: Added polygon moving/node deletion/drawing polygon holes features to annotate plugin

Dennis Nienhüser earthwings at gentoo.org
Thu May 29 20:54:52 UTC 2014 

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



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

    innerBoundaries() can never be empty here?



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

    Deselect
    
    Please change to title capitalization also (i.e. "Deselect All Nodes") here and below, see http://techbase.kde.org/Projects/Usability/HIG/Capitalization
    
    Would "Select all nodes" be useful as well?
    



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

    Maybe disable (or don't show at all) this one if no node is selected?



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

    Deselect



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

    <



src/plugins/render/annotate/AreaAnnotation.h
<https://git.reviewboard.kde.org/r/118405/#comment40878>

    const QPoint &point



src/plugins/render/annotate/AreaAnnotation.cpp
<https://git.reviewboard.kde.org/r/118405/#comment40876>

    This will become totally sweet when you extract it as a new method in GeoDataCoordinates with the signature
    
    GeoDataCoordinates GeoDataCoordinates::destination( qreal bearing, qreal distance ) const



src/plugins/render/annotate/AreaAnnotation.cpp
<https://git.reviewboard.kde.org/r/118405/#comment40877>

    Use GeoDataCoordinates::destination here as well then



src/plugins/render/annotate/AreaAnnotation.cpp
<https://git.reviewboard.kde.org/r/118405/#comment40875>

    Better do the check in screen coordinates. Otherwise you depend on zoom level and location on earth



src/plugins/render/annotate/GroundOverlayFrame.cpp
<https://git.reviewboard.kde.org/r/118405/#comment40872>

    unused



src/plugins/render/annotate/SceneGraphicsItem.h
<https://git.reviewboard.kde.org/r/118405/#comment40871>

    should be const correct, i.e.
    const GeoDataPlacemark *placemark() const;
    or
    GeoDataPlacemark *placemark();
    or both.


- Dennis Nienhüser


On May 29, 2014, 4:56 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118405/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 4:56 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> I added the following features to polygon editing:
> - moving the whole polygon. I had already submitted a review request for this, but Dennis mentioned some issues so I discarded it and added the changes to this patch. It is important to mention that this feature does not fully work - there are some problems when moving the polygon near the poles.
> - node selection and deletion. I increased the size of the bullets representing the nodes in order to ease their selection. The selected ones are painted with a different color. 
> - adding holes to polygons (more precisely, adding inner boundaries). For this I added a new option in the menu, "Add Polygon Hole". It works similar to drawing polygons: check it, draw the hole then un-check it so that it knows that you finished drawing the hole. Hopefully, when I will start working on the user interface, this will not remain like this - it will be more user-friendly.
> 
> Also, I added the following new actions (grouped into two kind of menus: polygon menu - which shows the options for the entire polygon and node menu which shows the options for the right clicked node):
> - select/unselect node: this comes to complete the node selection done by clicking using the left button of the mouse.
> - delete node: deletes the right-clicked node. If the polygon (or inner boundary ring) has less than 3 nodes after deletion, the whole polygon(inner boundary) will be removed. I think having only a line on the screen is not what a user may expect - but if you find it may have a purpose, it is easy to modify this. Also, here I dealt with the situation in which after deleting a node from the polygon's outer boundary, one of its inner boundaries would not be an inner boundary anymore (they would intersect). I used a QMessageBox warning to announce the user about this. Initially I tried to disable the action when this happened ("Delete node" and "Delete selected nodes") but this proved very inefficient in my opinion since the whole deletion should have been "simulated" before showing the menu. It is important to mention, also, that there are still some problems with this testing and I could not figure out so far what causes that: sometimes the function which tests if the polygon's outer boundary contains all the coordinates of its inner boundaries returns true even though it doesn't (or at least it doesn't look like from what is painted on the globe, e.g: http://imgur.com/a/KCoXO)
> 
> - unselect all nodes: this is an action from the polygon rmb menu. I think it its useful if you have too many nodes selected and you want to select other in order to delete them at the same time, for example.
> - delete selected nodes: it behaves similar to delete node, only that is deletes all selected ones.
> - remove polygon: removes the entire polygon.
> 
> I also intensively tried to clean the code and make it look more organized and better structured. I've also written some documentation to some classes and added many comments - I hope they don't bother; many of them I've written for me, to better understand the code, but may be useful for others too.
> 
> I'm really looking forward to seeing your reviews and issues, as well as potential recommendations for the features which are not fully working.
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/annotate/AnnotatePlugin.h 3d8b9b5 
>   src/plugins/render/annotate/AnnotatePlugin.cpp e1add48 
>   src/plugins/render/annotate/AreaAnnotation.h 105e2e7 
>   src/plugins/render/annotate/AreaAnnotation.cpp 0beff80 
>   src/plugins/render/annotate/CMakeLists.txt 387a92d 
>   src/plugins/render/annotate/EditGroundOverlayDialog.ui d25d74a 
>   src/plugins/render/annotate/GeoWidgetBubble.h 944f3e3 
>   src/plugins/render/annotate/GeoWidgetBubble.cpp 4837f96 
>   src/plugins/render/annotate/GroundOverlayFrame.h 4b1337d 
>   src/plugins/render/annotate/GroundOverlayFrame.cpp 30b1d0b 
>   src/plugins/render/annotate/PlacemarkTextAnnotation.h 492bd4b 
>   src/plugins/render/annotate/PlacemarkTextAnnotation.cpp 2d08ff7 
>   src/plugins/render/annotate/SceneGraphicsItem.h 3e9ea08 
>   src/plugins/render/annotate/SceneGraphicsItem.cpp a5d9da7 
>   src/plugins/render/annotate/SceneGraphicsTypes.h PRE-CREATION 
>   src/plugins/render/annotate/SceneGraphicsTypes.cpp PRE-CREATION 
>   src/plugins/render/annotate/TextEditor.cpp 6820330 
> 
> Diff: https://git.reviewboard.kde.org/r/118405/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

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

More information about the Marble-devel mailing list