<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102610/">http://git.reviewboard.kde.org/r/102610/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/102610/diff/1/?file=36119#file36119line60" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/GeoDataTreeModel.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class GeoDataTreeModel : public QAbstractItemModel</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">60</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QModelIndex</span> <span class="n">index</span><span class="p">(</span> <span class="n">GeoDataFeature</span><span class="o">*</span> <span class="n">feat</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line431" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/GeoDataTreeModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Qt::ItemFlags GeoDataTreeModel::flags ( const QModelIndex & index ) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">431</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">QModelIndex</span> <span class="n">GeoDataTreeModel</span><span class="o">::</span><span class="n">index</span><span class="p">(</span> <span class="n">GeoDataFeature</span> <span class="o">*</span><span class="n">feat</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Small style nitpick, please use full names for variables instead of abbreviations, even through the rest of the code</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line433" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/GeoDataTreeModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Qt::ItemFlags GeoDataTreeModel::flags ( const QModelIndex & index ) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">433</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">//This function recovers the QModelIndex of a given feature in the geodatatree structure.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line445" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/GeoDataTreeModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Qt::ItemFlags GeoDataTreeModel::flags ( const QModelIndex & index ) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">445</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">itup</span><span class="o">=</span><span class="k">dynamic_cast</span><span class="o"><</span><span class="n">GeoDataFeature</span><span class="o">*></span> <span class="p">(</span><span class="n">itup</span><span class="o">-></span><span class="n">parent</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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...</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/102610/diff/1/?file=36120#file36120line475" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/GeoDataTreeModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int GeoDataTreeModel::addFeature( GeoDataContainer* parent, GeoDataFeature* feature )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">475</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="p">{</span><span class="ew"> </span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">uho, tab got introduced here... we try to stay eol clean (my editor does it for me actually)</pre>
</div>
<br />
<p>- Thibaut</p>
<br />
<p>On September 14th, 2011, 10:57 a.m., Javier Becerra Elcinto wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Javier Becerra Elcinto.</div>
<p style="color: grey;"><i>Updated Sept. 14, 2011, 10:57 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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();
}
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/lib/GeoDataTreeModel.h <span style="color: grey">(eda4c53)</span></li>
<li>src/lib/GeoDataTreeModel.cpp <span style="color: grey">(2c17cfa)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102610/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>