<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://reviewboard.kde.org/r/4285/">http://reviewboard.kde.org/r/4285/</a>
     </td>
    </tr>
   </table>
   <br />





 <pre>The general direction of the patch looks fine,
but I get a linking error:

Linking CXX shared module ../../../../lib/GeoRendererPlugin.so                              
CMakeFiles/GeoRendererPlugin.dir/GeoRendererView.o: In function `Marble::GeoRendererView::renderIndex(QModelIndex&amp;)&#39;:
/home/bastian/devel/kde/marble/src/plugins/render/geodata/GeoRendererView.cpp:109: undefined reference to `Marble::GeoDataTypes::GeoDataDocumentType&#39;
collect2: ld returned 1 exit status

There also some minor issues I see ...:</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="/r/4285/diff/2/?file=29849#file29849line33" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.h</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
   <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
    <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">33</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"><span class="cm">     * Creates a new GeoDataModel.</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">No, it&#39;s a GeoDataTreeModel</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="/r/4285/diff/2/?file=29850#file29850line52" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
   <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">int GeoDataTreeModel::rowCount( const QModelIndex &amp;parent ) 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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
    <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">52</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"><span class="c1">//    qDebug() &lt;&lt; &quot;rowCount&quot;;</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">Why don&#39;t you use mDebug() there?</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="/r/4285/diff/2/?file=29858#file29858line75" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;">class GEODATA_EXPORT GeoDataContainer : public GeoDataFeature</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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
    <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">75</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">    <span class="n">QVector</span><span class="o">&lt;</span><span class="n">GeoDataFeature</span><span class="o">&gt;</span> <span class="n">features</span><span class="p">()</span> <span class="k">const</span><span class="p">;</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">I&#39;m not sure here.
What is the reason for still having the features() method?
If we really need this, we should do a documentation of both and clearly describe the difference.

Is this the typical Qt naming scheme?</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="/r/4285/diff/2/?file=29858#file29858line93" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;">class GEODATA_EXPORT GeoDataContainer : public GeoDataFeature</pre></td>

  </tr>
 </tbody>



 
 




 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">92</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"><span class="c1"><span class="hl">//</span>   <span class="hl"> virtual</span> int childPosition( GeoData<span class="hl">Object* </span>);</span></pre></td>
    <th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">93</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">    <span class="kt">int</span> <span class="n">childPosition</span><span class="p">(</span> <span class="n">GeoDataFeature</span> <span class="o">*</span><span class="n">child</span><span class="p">);</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">Are you sure that you can remove the virtual 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="/r/4285/diff/2/?file=29869#file29869line191" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;">class GEODATA_EXPORT GeoDataPlacemark: public GeoDataFeature</pre></td>

  </tr>
 </tbody>





 
 


 <tbody>

  <tr>
    <th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">191</font></th>
    <td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="nl">private:</span></pre></td>
    <th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
    <td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">Why are you commenting out the private thing? 
That does not look reasonable.

If there is a reasonable reason, you should comment this.</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="/r/4285/diff/2/?file=29876#file29876line107" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.cpp</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </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; white-space: pre-wrap; word-wrap: break-word;">void GeoRendererView::renderIndex( QModelIndex &amp;index )</pre></td>

  </tr>
 </tbody>



 
 




 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">106</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">    <span class="k">if</span><span class="p">(</span> <span class="o">!</span><span class="p">(</span> <span class="k">dynamic_cast</span><span class="o">&lt;</span><span class="n">GeoDataFeature</span><span class="o">*&gt;</span><span class="p">(</span> <span class="n">indexObject</span> <span class="p">)</span> <span class="o">&amp;&amp;</span> <span class="k">dynamic_cast</span><span class="o">&lt;</span><span class="n">GeoDataFeature</span><span class="o">*&gt;</span><span class="p">(</span> <span class="n">indexObject</span> <span class="p">)</span><span class="o">-&gt;</span><span class="n">isVisible</span><span class="p">()</span> <span class="p">)</span> <span class="p">)</span> <span class="k">return</span><span class="p">;</span></pre></td>
    <th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">107</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">    <span class="p">{</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em">Again a minor thing.
You don&#39;t use spaces inside the parenthesis and the opening brace should be in the same line with the if.</pre>
</div>
<br />



<p>- Bastian</p>


<br />
<p>On June 27th, 2010, 9:46 p.m., Thibaut Gridel wrote:</p>




<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/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 Thibaut Gridel.</div>


<p style="color: grey;"><i>Updated 2010-06-27 21:46:09</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;">providing a (naive) pointer-based api for GeoDataContainer, and test case which shows early breaks...

Toggle between the two &quot;document.append(folder);&quot; shows how brittle the api still is.</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;">Run TestGeoData :(</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>/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/FileManager.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/FileManager.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.h <span style="color: grey">(PRE-CREATION)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/GeoDataTreeModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/KmlFileViewItem.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/KmlFileViewItem.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/MarbleGeometryModel.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/MarbleGeometryModel.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataContainer_p.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFolder.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataGeometry.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataMultiGeometry_p.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataObject.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/geodata/writers/kml/KmlDocumentTagWriter.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererPlugin.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.h <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/src/plugins/render/geodata/GeoRendererView.cpp <span style="color: grey">(1143093)</span></li>

 <li>/trunk/KDE/kdeedu/marble/tests/TestGeoData.cpp <span style="color: grey">(1143093)</span></li>

</ul>

<p><a href="http://reviewboard.kde.org/r/4285/diff/" style="margin-left: 3em;">View Diff</a></p>




  </td>
 </tr>
</table>








  </div>
 </body>
</html>