[Marble-devel] Review Request 119172: Annotate Plugin refactoring. Added the 'Adding Nodes' feature.

Wed Jul 9 21:02:51 UTC 2014

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

(Updated July 10, 2014, 12:02 a.m.)

Review request for Marble, Dennis Nienhüser and Torsten Rahn.

Changes
-------

Updated the patch according to idis' suggestion - to change the name of two methods: itemChanged (now dealWithItemChange) and stateChanged(dealWithStateChange).

Repository: marble

Description
-------

As the title says, this patch includes two major 'changes':
- code refactoring in Annotate Plugin; I don't refer to the class itself but to the whole plugin. I'll explain next the changes.
- new feature added, 'Adding Nodes' which now allows the users to add nodes to an already drawn polygon. To add new nodes, simply check the 'Adding Nodes' option and hover the middle of each polygon's line (both from its outer boundary and its inner boundaries). Then click the 'virtual node' which appears and you will be able to adjust this new node to the position you want. Click once again and it sticks to that position and then you can continue adding new nodes.

So, what is the refactoring all about?
Well, the problem was that the code became pretty hard to understand (especially by possible newcomers) and hard to extend, due to the way I have previously implemented some things; e.g. the way I stored the selected nodes, using a list of indexes proved not to be the best option as more and more features were added. And there were much more aspects similar to this. 

So, what I thought is that it would be nice that each option we have so far in our Annotate Plugin (Add Polygons, Add Polygons Hole, Add Placemark, etc) to be represented as a 'state' of our Editing Mode, since there could not be two of these options selected at the same time (or if it could, it should not, because it would cause redundancies); this means, you cannot be adding a placemark and addins nodes to a polygon at the same time (with these being said, what I let as a TODO is to not allow having more than one of these options selected - so far it is possible to check more of them and I cannot guarantee what will happen). So I added an enum in the base class, SceneGraphicsItem, and getter and setter for the state of the item and each time the state is changed, all drawn SceneGraphicsItems will be 'announced' about the change of state (see AnnotatePlugin::announceStateChanged). It makes possible for each SceneGraphicsItem to decide what to do on each event, depending on its STATE. This is actually what I did next in AreaAnnotation. 
Another change which eased my work and hopefully will ease my future work on the plugin is the PolygonNode class which I added. What I thought is that it would be nice to keep in some object all information regarding a node (its region as well as other options, like if this is selected or not) - so this is what I did. The next problem was that, in the previous version of AreaAnnotation, the regions were created at each AreaAnnotation::paint call, so I needed to avoid this in order to not lose the information in each Node (the flags). So what I did is I only create the PolygonNodes at the first ::paint call and then only update them, taking into consideration they are in the same order in my lists as they are in polygon->outerBoundary() and polygon->innerBoundaries().
One more change which came next was to not use anymore the SceneGraphicsItem::regions() and setRegions() but keep these regions in each concrete class derived from it, since each class may organize differently their regions and would be hard to keep all of them in only one list. I replaced these two methods with a new one, containsPoint( const QPoint& ) which is re-implemented in each concrete class according to its regions. This gets nice together with the changes I mentioned above, because, e.g. in AreaAnnotation, this method takes into consideration different regions on different states.

I don't want it to become a TLDR so I will let you browse the code and see the changes and then come back with suggestions or aspects which does not seem good to you. ALSO, apart from the internal quality, I'm expecting review on the external quality: how does each feature feel so far and what would you like to see added/remove/changed. Thanks!

Known TODOs:
- moving polygon around poles;
- the problem with tessellation: when I test if the polygon has a valid shape (its outer boundary contains all its inner boundary nodes), the GeoDataLinearRing::contains does not take into consideration the tessellation flag and thus leads to some weird results sometimes. But this is not very annoying - could be documented.
- adding icons for the new entries in the View menu (this will be solved soon, I'm working on them);
- maybe virtual nodes to be shown everywhere on a line between any two existing nodes? This would imply to add as a region each 'line' + some offset. Any suggestion how to do this?
- more friendly user interface - all this stuff with 'states' will become much more intuitive when this will be done (e.g. the toolbar buttons in Marble Qt);

Later Edit: I made a blog post about the changes. You can find it here if you are interested: http://calincruceru.wordpress.com/2014/07/08/editing-mode-for-polygons-gsoc-project-progress/

Diffs (updated)
-----

  src/plugins/render/annotate/AnnotatePlugin.h 2aa9323 
  src/plugins/render/annotate/AnnotatePlugin.cpp 0e5546f 
  src/plugins/render/annotate/AreaAnnotation.h 83fd066 
  src/plugins/render/annotate/AreaAnnotation.cpp 047f0f4 
  src/plugins/render/annotate/EditGroundOverlayDialog.cpp 735b1aa 
  src/plugins/render/annotate/EditPolygonDialog.h 2ea9962 
  src/plugins/render/annotate/EditPolygonDialog.cpp 09bb8c2 
  src/plugins/render/annotate/GeoWidgetBubble.h 859f26c 
  src/plugins/render/annotate/GeoWidgetBubble.cpp 8ea0bfe 
  src/plugins/render/annotate/GroundOverlayFrame.h f56abcd 
  src/plugins/render/annotate/GroundOverlayFrame.cpp c9a6e6c 
  src/plugins/render/annotate/PlacemarkTextAnnotation.h 05d506b 
  src/plugins/render/annotate/PlacemarkTextAnnotation.cpp eff8a32 
  src/plugins/render/annotate/SceneGraphicsItem.h b0b02c3 
  src/plugins/render/annotate/SceneGraphicsItem.cpp 7891abe 
  src/plugins/render/annotate/SceneGraphicsTypes.h 1a35ee6 
  src/plugins/render/annotate/SceneGraphicsTypes.cpp 378bfce 
  src/plugins/render/annotate/TextEditor.cpp dd3706d 
  tests/TestEquality.cpp 67e8a9e 

Diff: https://git.reviewboard.kde.org/r/119172/diff/

Testing
-------

I spent some time testing each feature carefully, but since I re-wrote the AreaAnnotation from scratch, it is possible to not be bug free, so please tell me if you find some weird behaviour or you expect something to behave differently.

Thanks,

Cruceru Calin-Cristian

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