[Marble-devel] Review Request 118885: Added 'Merging Nodes' feature

[Cruceru Calin-Cristian]

> On June 25, 2014, 3:23 p.m., Torsten Rahn wrote:
> > src/plugins/render/annotate/AnnotatePlugin.h, line 160
> > <https://git.reviewboard.kde.org/r/118885/diff/1/?file=283713#file283713line160>
> >
> >     Maybe this should get called "handle" or "process" instead of "dealwith"

Right, is sounds better.


> On June 25, 2014, 3:23 p.m., Torsten Rahn wrote:
> > src/plugins/render/annotate/AnnotatePlugin.cpp, line 387
> > <https://git.reviewboard.kde.org/r/118885/diff/1/?file=283714#file283714line387>
> >
> >     I guess the m_movedItem is deleted elsewhere? Could you add a comment on this? :)

Here m_movedItem is just pointing to an element from m_graphicsItem so we are not leaking memory here, because just three lines after this it is called qDeleteAll( m_graphicsItem ). However, I added a comment to be more clear.


- Cruceru


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


On June 22, 2014, 11:09 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118885/
> -----------------------------------------------------------
> 
> (Updated June 22, 2014, 11:09 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Added Merging Nodes option on polygons. 
> I designed this new feature as a new "state" of the editing mode. This means that one cannot merge nodes and draw polygon holes at the same time since these are two different states. Initially I tried to implement a basic functionality by adding a new option within the RMB menu on polygons but it would have been pretty hard to decide which nodes to merge (the two selected ones? And what if I need to have more than two marked as selected?).
> So, to enter this new 'Merging Nodes' state simply check the 'Merging Nodes' entry in the View menu (also make sure that there is only one option checked from the polygons editing tools - I let as a TODO not allow having more than one checked, but this is not as trivial as it may sound, since I also have to take into consideration the other editing mode tools like adding placemarks, etc). Then simply start clicking the nodes you want to merge in pairs of two. I also treated all special cases like trying to merge a node from the outer boundary with a node from the polygon's inner boundary or to merge two nodes from two different inner boundaries (this will result in a warning). The visual effect when merging nodes is basic so far: I only draw differently the first selected node (I am waiting suggestions on how would you like this merging to "look", visually speaking). 
> One important thing to mention is that I did not restrict the merging on neighbor nodes because I thought that one may also find useful how it works on nodes which are not neighbor.
> 
> 
> This patch also contains a new design for handling events in Annotate Plugin class which, in my opinion, is more intuitive and easily extensible (I even experienced this while implementing 'Merging Nodes'). To describe this new approach I have to say first that I was inspired by Adam's changes in MarbleInputHandler. Since AnnotatePlugin::eventFilter would have become larger and larger as new features are added, something for sure should have been modified. Tell me if you think the split technique I approached is a good solution to this.
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/annotate/AnnotatePlugin.h 0b426a7 
>   src/plugins/render/annotate/AnnotatePlugin.cpp d6e5e8a 
>   src/plugins/render/annotate/AreaAnnotation.h d349a3c 
>   src/plugins/render/annotate/AreaAnnotation.cpp 3952a91 
>   src/plugins/render/annotate/GroundOverlayFrame.h 5037430 
>   src/plugins/render/annotate/PlacemarkTextAnnotation.h cdc9a0e 
> 
> Diff: https://git.reviewboard.kde.org/r/118885/diff/
> 
> 
> Testing
> -------
> 
> I tested most of the possibilities: simple polygon with only outer boundary, polygon with an inner boundary, polygon with multiple inner boundaries, polygon with selected nodes on the outer boundary, polygon with selected nodes on each inner boundary, etc. In all my tests it worked as expected. However, the chances of crash increase proportionally with the number of features added to this edit mode, so some more testing is always good.
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

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