[Marble-devel] Review Request 118076: Adapted Ground Overlays Editing implementation to master and fixed its issues

Tue May 13 10:33:08 UTC 2014

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

This review has been submitted with commit 47b087ad939b2f4ea70b06da74dd19ad5c097637 by Cruceru Calin-Cristian to branch master.

- Commit Hook

On May 10, 2014, 6:35 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118076/
> -----------------------------------------------------------
> 
> (Updated May 10, 2014, 6:35 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> I started from this existing patch submitted by Adrian Draghici who worked on implementing (alongside other things) the overlay editing within marble, during GSoC 2013: https://git.reviewboard.kde.org/r/112454/. Most of it was simply to adapt since there were little changes to the modified files since last year.
> 
> The harder part was after I finished fully adapting every piece of code and it even compiled. It was getting a segmentation error when trying to modify an overlay after it has been projected on the map. I finally found the problem (I wrote a comment within the code, where the missing line was) and it seems to work pretty good now. Or at least there are no more SIGSEGVs.
> 
> One thing I am especially waiting opinions is the current functionality of moving an overlay. At the moment, if one wants to move an overlay, if they click the overlay, they will be prompted with the EditGroundOverlayDialog. When exiting from this dialog, the overlay will be selected, so this is the moment one could drag the overlay to another position on the map or to right click it and catch the event which shows the two options: remove and edit. So, here are 2 issues:
> - it is pretty annoying to cancel the EditGroundOverlayDialog every time you want to move the overlay by dragging it.
> - the rmbMenu which is poped-up when you right-click the selected overlay shows the edit options which at least at the moment is useless and redundant since you could obtain the same result by simply clicking the overlay.
> 
> Two options regarding these issues would be, in my opinion:
> - since the EditGroundOverlayDialog includes the possibility to modify the coordinates of the overlay, the dragging could be replaced by simply introducing the coordinates you want to move the overlay to (so to not catch the mouse move event anymore). At the same time, the usual user may find easier to drag and drop the overlay and since we have to look at this from the user's point of view, the above solution may not be suitable. So I'm waiting from you to tell me how you, as a Marble user, would find easily and intuitively to both move the overlay by drag and droping and to be prompted by the dialog (to change name, description, etc) and, why not, some implementation details.
> - for the rmbMenu, if there are no future plans of extending its actions, we may totally remove it and add another event (for example pressing delete, or some other key) to remove the overlay (which, by the way, is already implemented: View->Remove item).
> 
> To conclude, I'm waiting for both an overall opinion (since the whole implementation will be wrapped up within an Dockwidget or some other menu, alongside Polygons Editing tool on which I will proceed to work, so there will still be modifications) as well as a more detailed one, so I can easily fix the mentioned issues.
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-ui/icons/draw-overlay.png PRE-CREATION 
>   src/apps/marble-ui/marble.qrc 39ad981 
>   src/lib/marble/FileManager.h ae648f7 
>   src/lib/marble/FileManager.cpp e5551a6 
>   src/lib/marble/FileViewWidget.cpp 659a9d9 
>   src/lib/marble/MarbleMap.h 7019bfa 
>   src/lib/marble/MarbleMap.cpp 9859c02 
>   src/lib/marble/MarbleWidget.h 067e9ce 
>   src/lib/marble/MarbleWidget.cpp f6baab4 
>   src/lib/marble/geodata/writers/kml/KmlOverlayTagWriter.cpp ccc13c0 
>   src/lib/marble/layers/TextureLayer.h 90042cb 
>   src/plugins/render/annotate/AnnotatePlugin.h c4d1530 
>   src/plugins/render/annotate/AnnotatePlugin.cpp 447e154 
>   src/plugins/render/annotate/AreaAnnotation.cpp 7db109b 
>   src/plugins/render/annotate/CMakeLists.txt ac8b94e 
>   src/plugins/render/annotate/EditGroundOverlayDialog.h PRE-CREATION 
>   src/plugins/render/annotate/EditGroundOverlayDialog.cpp PRE-CREATION 
>   src/plugins/render/annotate/EditGroundOverlayDialog.ui PRE-CREATION 
>   src/plugins/render/annotate/GeoWidgetBubble.cpp f7acde7 
>   src/plugins/render/annotate/GroundOverlayFrame.h PRE-CREATION 
>   src/plugins/render/annotate/GroundOverlayFrame.cpp PRE-CREATION 
>   src/plugins/render/annotate/PlacemarkTextAnnotation.h 996a2b2 
>   src/plugins/render/annotate/PlacemarkTextAnnotation.cpp bbfecea 
>   src/plugins/render/annotate/SceneGraphicsItem.h 48e965b 
>   src/plugins/render/annotate/SceneGraphicsItem.cpp 2c7f92b 
>   src/plugins/render/annotate/TextEditor.h 3ae29a2 
>   src/plugins/render/annotate/TextEditor.cpp 10d2ac6 
> 
> Diff: https://git.reviewboard.kde.org/r/118076/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Draw overlay action picture
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/10/48689c21-49ab-4bae-b282-9204fa0b2b4e__draw-overlay.png
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

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