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

Tue May 13 10:33:11 UTC 2014

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

(Updated May 13, 2014, 10:33 a.m.)

Status
------

This change has been marked as submitted.

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/c8c80d46/attachment-0001.html>