[Marble-devel] Review Request: Added addFeature and removeFeature support to GeoDataTreeModel

[Thibaut Gridel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102610/#review7224
-----------------------------------------------------------

Ship it!


OK, all looks perfectly good!
Please commit, Guillaume is getting very interested...

- Thibaut Gridel


On Sept. 30, 2011, 3:18 p.m., Javier Becerra Elcinto wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102610/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2011, 3:18 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> 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/diff
> 
> 
> Testing
> -------
> 
> Files "PlacemarkInserter.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 Becerra Elcinto
> 
>

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