[Marble-devel] Review Request: Added addFeature and removeFeature support to GeoDataTreeModel
Javier Becerra Elcinto
javier at auva.es
Wed Sep 14 19:45:05 UTC 2011
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > 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.
Thanks for your feedback!
See my comments below
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > src/lib/GeoDataTreeModel.h, line 60
> > <http://git.reviewboard.kde.org/r/102610/diff/1/?file=36119#file36119line60>
> >
> > 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.
I chose to limit the function to GeoDataFeature as a matter of efficiency; features are restricted to be children of containers and containers have a "childPosition" function (required to build the tree top-down). GeoDataObjects do not keep track of their children (iirc), and you would need to treat some (many?) classes derived from GeoDataObject individually.
A Feature seemed atomic enough to update a model with minimum impact.
If we could build a QModelIndex without going up and down the tree that would also ease the task.
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > src/lib/GeoDataTreeModel.cpp, line 431
> > <http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line431>
> >
> > Small style nitpick, please use full names for variables instead of abbreviations, even through the rest of the code
Sorry for that one, I thought I had corrected it.
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > src/lib/GeoDataTreeModel.cpp, line 433
> > <http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line433>
> >
> > 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.
OK
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > src/lib/GeoDataTreeModel.cpp, line 445
> > <http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line445>
> >
> > 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...
Ok, I tend to double-check security rather than performance, but you are right that it is a sensitive piece of code in that sense.
I don't like the nodeType testing that much but it seems to be working ok (I read someone suggesting to change it to an enum, seems like a good idea).
> On Sept. 14, 2011, 6:52 p.m., Thibaut Gridel wrote:
> > src/lib/GeoDataTreeModel.cpp, line 475
> > <http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line475>
> >
> > uho, tab got introduced here... we try to stay eol clean (my editor does it for me actually)
Sorry, I thought the editor of QtCreator handled that automatically. I will double check next time.
- Javier
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102610/#review6509
-----------------------------------------------------------
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/fa3a58e4/attachment-0001.html>
More information about the Marble-devel
mailing list