----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119172/#review61885 -----------------------------------------------------------
Looks nice - please give me a day for a proper review :) - Torsten Rahn On July 8, 2014, 8:38 a.m., Cruceru Calin-Cristian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119172/ > ----------------------------------------------------------- > > (Updated July 8, 2014, 8:38 a.m.) > > > Review request for Marble, Dennis Nienhüser and Torsten Rahn. > > > 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); > > > Diffs > ----- > > 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 > 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 > > 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 > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
