[Marble-devel] Review Request: Added addFeature and removeFeature support to GeoDataTreeModel
Thibaut Gridel
tgridel at free.fr
Wed Sep 14 18:52:25 UTC 2011
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102610/#review6509
-----------------------------------------------------------
Very nice patch!
I haven't applied nor tried the code yet, only provided some nitpicks from the coding style and intent category.
It is the way to go for sure.
src/lib/GeoDataTreeModel.h
<http://git.reviewboard.kde.org/r/102610/#comment5758>
Your methods assume a GeoDataFeature is the base class that could ever be manipulated by the tree.
Historically I had gone even in Geometries in case of MultiGeometries (see for example parent() code), so If we still want to go there we should use a GeoDataObject i fear.
Maybe addFeature and removeFeature are enough to manipulate placemarks though.
As a todo for another time, appending a geometry in a multigeo would need handling from the model too.
src/lib/GeoDataTreeModel.cpp
<http://git.reviewboard.kde.org/r/102610/#comment5754>
Small style nitpick, please use full names for variables instead of abbreviations, even through the rest of the code
src/lib/GeoDataTreeModel.cpp
<http://git.reviewboard.kde.org/r/102610/#comment5755>
The first comment may go in the header above the method name, doxygen style, so that documentation will pick it.
The other comments for internal details are fine here.
src/lib/GeoDataTreeModel.cpp
<http://git.reviewboard.kde.org/r/102610/#comment5756>
argh, dynamic cast has proven very bad performance wise in this perfo intensive class. Could you replace with other cast or even use the nodeType() testing?
Mimetism has it that even if the code is not perfo sensitive, someone will copy the dynamic_cast idea somewhere bad...
src/lib/GeoDataTreeModel.cpp
<http://git.reviewboard.kde.org/r/102610/#comment5759>
uho, tab got introduced here... we try to stay eol clean (my editor does it for me actually)
- Thibaut
On Sept. 14, 2011, 10:57 a.m., Javier Becerra Elcinto wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102610/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2011, 10:57 a.m.)
>
>
> Review request for Marble.
>
>
> Summary
> -------
>
> GeoDataTreeModel allows for insertion of a GeoDataDocument using the function addDocument(GeoDataDocument*). However it is not possible to modify an existing document (i.e. by adding or removing a feature) while keeping the model/view synchonised other than calling the GeoDataTreeModel::update() function, which updates the whole treemodel and demands a lot of ressources.
> Adding new addFeature and removeFeature allows the modification of the tree while keeping the view automatically updated.
> GeoDataTreeModel::addDocument and GeoDataTreeModel::removeDocument are modified to call addFeature and removeFeature, minimising the code modifying the treemodel.
>
> This patch has been proved to work in single-threaded mode, however marblewidget crashes on KDescendantsProxyModel::mapFromSource(const QModelIndex &sourceIndex) when a new feature/document is added from a thread different from main. This behaviour is independent of this patch, as it can be reproduced in unpatched marblewidget calling GeoDataTreeModel::addDocument from another thread. The test code below can be used to test this bug uncommenting the last two lines of code of main.cpp.
>
>
> Diffs
> -----
>
> src/lib/GeoDataTreeModel.h eda4c53
> src/lib/GeoDataTreeModel.cpp 2c17cfa
>
> Diff: http://git.reviewboard.kde.org/r/102610/diff
>
>
> Testing
> -------
>
> Files "PlacemarkInserte.h" and "main.cpp":
>
> #ifndef PLACEMARKINSERTER_H
> #define PLACEMARKINSERTER_H
>
> //Marble
> #include "GeoDataContainer.h"
> #include "GeoDataPlacemark.h"
> #include "MarbleWidget.h"
> #include "MarbleModel.h"
> #include "GeoDataTreeModel.h"
>
> //Qt
> #include <QtCore/QTimer>
> #include <QtCore/QDebug>
>
> class PlacemarkInserter:public QObject
> {
> Q_OBJECT
> Marble::GeoDataContainer*folder;
> Marble::MarbleWidget *marblewidget;
> float lat;
> float lon;
>
> int index;
> QString prefix;
> QTimer timer;
>
> bool removeprev;
> Marble::GeoDataPlacemark *prev;
> public:
> PlacemarkInserter(float lon0, float lat0, int interval, QString pr, Marble::GeoDataContainer* f, Marble::MarbleWidget *mw=NULL ): lat(lat0), lon(lon0), prefix(pr), folder(f), marblewidget(mw), index(2), prev(NULL), removeprev(false)
> {
> timer.setInterval(interval);
> timer.setSingleShot(false);
> QObject::connect(&timer,SIGNAL(timeout()),this,SLOT(addNewPlacemark()));
>
> }
> public slots:
> void setLatitude( float lat0 ) {lat=lat0;}
> void setLongitude( float lon0 ) {lon=lon0;}
> void setRemovePrevious( bool r ) {removeprev=r;}
> void setInterval( int i ) {timer.setInterval(i);}
> void start() { timer.start(); }
> void stop() { timer.stop(); }
>
> void addNewPlacemark()
> {
> qDebug()<<"Adding placemark "<<prefix<<index;
>
> Marble::GeoDataPlacemark *myplacemark=new Marble::GeoDataPlacemark;
> myplacemark->setName(QString("%1 %2").arg(prefix).arg(index++));
> myplacemark->setCoordinate( lon+index*0.01, lat,0,Marble::GeoDataPoint::Degree);
>
> if( marblewidget )
> {
> if( removeprev && prev )
> {
> marblewidget->model()->treeModel()->removeFeature( prev );
> delete prev;
> }
> marblewidget->model()->treeModel()->addFeature( folder, myplacemark );
> prev=myplacemark;
> }
>
> }
>
> };
>
> #include <QThread>
> class PlacemarkInserterTh: public QThread
> {
> float lon;
> float lat;
> int interval;
> Marble::GeoDataContainer *folder;
> Marble::MarbleWidget *marblewidget;
> QString prefix;
>
> public:
> PlacemarkInserterTh(float lon0, float lat0,int interval0, QString pr,Marble::GeoDataContainer *f, Marble::MarbleWidget *mw): lon(lon0),lat(lat0), interval(interval0),folder(f), marblewidget(mw), prefix(pr) {}
> void run() { PlacemarkInserter inserter(lon,lat,interval,prefix, folder, marblewidget ); inserter.start(); this->exec();}
> };
>
>
> #endif // PLACEMARKINSERTER_H
>
>
> //////////////////////////////////////////////7
> //main.cpp
>
> #include "PlacemarkInserter.h"
>
> //Marble
> #include "GeoDataDocument.h"
> #include "GeoDataFolder.h"
> #include "GeoDataPlacemark.h"
> #include "MarbleWidget.h"
> #include "FileViewWidget.h"
>
> //Qt
> #include <QtGui/QApplication>
> #include <QDebug>
> #include <QTextCodec>
>
>
> int main(int argc, char** argv)
> {
> QApplication app(argc,argv);
>
>
> QTextCodec::setCodecForCStrings(QTextCodec::codecForName("UTF-8"));
> QTextCodec::setCodecForTr(QTextCodec::codecForName("UTF-8"));
>
> //Creating a GeoDataDocument from scratch:
>
> Marble::GeoDataDocument *mydoc=new Marble::GeoDataDocument();
>
> mydoc->setFileName("mydocument.kml"); //Will identify the document internally, must be unique
> mydoc->setName("My_GeoDataDocument"); //Name for visualisation in MarbleWidget
>
> Marble::GeoDataFolder *myfolder=new Marble::GeoDataFolder;
> myfolder->setName("My first folder"); //(setName from base class GeoDataFeature, inherited from GeoDataContainer)
>
> mydoc->append(myfolder);
>
> Marble::MarbleWidget marblewidget;
> marblewidget.setMapThemeId("earth/openstreetmap/openstreetmap.dgml");
> marblewidget.show();
>
> Marble::FileViewWidget fileviewwidget;
> fileviewwidget.setMarbleWidget(&marblewidget);
> fileviewwidget.show();
>
> //addDocument now issues a call to addFeature
> int idx=marblewidget.model()->treeModel()->addDocument(mydoc);
>
> qDebug()<<"Added document"<<idx;
>
>
> PlacemarkInserter inserter0(-2.45,42.465278,250,"Test",myfolder, &marblewidget );
> inserter0.setRemovePrevious(true); //tests new removeFeature functionality
> inserter0.start();
>
> PlacemarkInserter inserter1(-2.48, 42.465278 , 500 , "Test2" , mydoc , &marblewidget );
> inserter1.setRemovePrevious(false);
> inserter1.start();
>
>
> //Uncomment next two lines if you want to see marblewidget crash when the treemodel is modified from
> // a thread other than the main one. This bug was already present before the addFeature patch;
> // calling addDocument from another thread also caused the application to crash.
> //PlacemarkInserterTh crash_on_multithread(-2.55, 42.465278 , 500 , "Crash" , myfolder , &marblewidget );
> //crash_on_multithread.start();
>
> return app.exec();
> }
>
>
> Thanks,
>
> Javier
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20110914/79751825/attachment-0001.html>
More information about the Marble-devel
mailing list