<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2010, 7:01 p.m., <b>Bastian Holst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 &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() << "rowCount";</span></pre></td>
</tr>
</tbody>
</table>
<pre>Why don't you use mDebug() there?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">Fixed!</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2010, 7:01 p.m., <b>Bastian Holst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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"><</span><span class="n">GeoDataFeature</span><span class="o">></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>I'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>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">I kept the old API because I didn't want to force the change everywhere at the same time.
I expect to drop this in next row of patches, because the code is suboptimal to recreate a QVector on demand.
Wrt naming, my change was to turn the return out of a QVector& because I'm rebuilding it... There is some code still using this.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2010, 7:01 p.m., <b>Bastian Holst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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>Are you sure that you can remove the virtual here?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">I think so. The original uncommented use of childPosition was to have a virtual "do nothing" implementation in GeoDataObject, and have that carry for all GeoData.
It seems quite wrong, because only 2 classes (and their inherited) need children, namely GeoDataContainer and GeoDataMultiGeometry. I don't expect a subclass to reimplement the code, so dropped the virtual.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2010, 7:01 p.m., <b>Bastian Holst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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>Why are you commenting out the private thing?
That does not look reasonable.
If there is a reasonable reason, you should comment this.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">I did that for the sake of testing into TestGeoData that p() members survived their contorsion through containers. It used to break big time!!! Now that the pointer-based api is in place, and the test proved to work, I am bringing it back to private (and drop test cases).
The same happened in GeoDataFolder.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2010, 7:01 p.m., <b>Bastian Holst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 &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"><</span><span class="n">GeoDataFeature</span><span class="o">*></span><span class="p">(</span> <span class="n">indexObject</span> <span class="p">)</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">indexObject</span> <span class="p">)</span><span class="o">-></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>Again a minor thing.
You don't use spaces inside the parenthesis and the opening brace should be in the same line with the if.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">Will edit in this file to match surrounding code.
However this particular class needs a bigger lifting, because the determination of the root document to resolve styling is a gross hack.
I deemed the patch big enough to stop there ;)</pre>
<br />
<p>- Thibaut</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 "document.append(folder);" 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>