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

[Javier Becerra Elcinto]

-----------------------------------------------------------
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.


Changes
-------

Modified according to comments from Thibaut Gridel.
Declared the add/remove functions as slots.
Added signals added(GeoDataObject *) and removed(GeoDataObject *).


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 (updated)
-----

  src/lib/GeoDataTreeModel.h eda4c53 
  src/lib/GeoDataTreeModel.cpp 2c17cfa 

Diff: http://git.reviewboard.kde.org/r/102610/diff/diff


Testing (updated)
-------

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/20110930/786e25af/attachment-0001.html>