<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://svn.reviewboard.kde.org/r/5902/">http://svn.reviewboard.kde.org/r/5902/</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;">Some of my comments may address code that was just moved, not added.
Do you plan to commit that to 4.6 or 4.7?
I haven't tested the code yet, will do that tomorrow.</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41477#file41477line848" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.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 MARBLE_EXPORT MarbleMap : public QObject</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">848</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">TextureLayer</span> <span class="o">*</span><span class="n">textureLayer</span><span class="p">()</span> <span class="k">const</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;">Preferably returning a pointer to a const object -- or the method shouldn't be const</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line87" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.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; "></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">87</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">GeoDataObject</span> <span class="o">*</span><span class="n">object</span> <span class="o">=</span> <span class="k">static_cast</span><span class="o"><</span><span class="n">GeoDataObject</span><span class="o">*></span><span class="p">(</span> <span class="n">model</span><span class="o">-></span><span class="n">dataFacade</span><span class="p">()</span><span class="o">-></span><span class="n">treeModel</span><span class="p">()</span><span class="o">-></span><span class="n">index</span><span class="p">(</span><span class="mi">0</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">QModelIndex</span><span class="p">()).</span><span class="n">internalPointer</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;">Is it safe with that little error checking? index may be invalid, internal pointer null, dynamic_cast may return 0. I'd prefer some Q_ASSERTs at least.</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line208" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.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; ">void MarbleMapPrivate::paintGround( GeoPainter &painter, QRect &dirtyRect )</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">208</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">showPlaces</span><span class="p">,</span> <span class="n">showCities</span><span class="p">,</span> <span class="n">showTerrain</span><span class="p">,</span> <span class="n">showOtherPlaces</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;">Must be initialized. Not all code paths of propertyValue() ensure that the value is set, so you may end up using them unitialized later.
</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line379" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.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; ">int MarbleMap::radius() 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">377</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QTimer</span><span class="o">::</span><span class="n">singleShot</span><span class="p">(</span> <span class="mi">0</span><span class="p">,</span> <span class="n">d</span><span class="o">-></span><span class="n">m_textureLayer</span><span class="p">.</span><span class="n">tileLoader</span><span class="p">(),</span> <span class="n">SLOT</span><span class="p">(</span> <span class="n">update</span><span class="p">()</span> <span class="p">)</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;">What's the reason to use the timer here? Any threads involved?</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41482#file41482line1083" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.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; ">private Q_SLOTS:</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">1078</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">TextureLayer</span> <span class="o">*</span><span class="n">textureLayer</span><span class="p">()</span> <span class="k">const</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;">Foo const * const() or Foo * () as above</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://svn.reviewboard.kde.org/r/5902/diff/1/?file=41482#file41482line1086" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.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; ">private Q_SLOTS:</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">1081</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">MeasureTool</span> <span class="o">*</span><span class="n">measureTool</span><span class="p">()</span> <span class="k">const</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;">Foo const * const() or Foo * () as above</pre>
</div>
<br />
<p>- Dennis</p>
<br />
<p>On November 19th, 2010, 4:07 p.m., Bernhard Beschow wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.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 Bernhard Beschow.</div>
<p style="color: grey;"><i>Updated 2010-11-19 16:07:35</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;">Motivation
==========
The whole painting should be implemented in the view rather than in the model because it decouples data and its representation. This allows for having different graphics backends (hello OpenGL!) while having a common interface to the data.
Implementation
==============
MarbleMap is now the owner of all painting objects, e.g. the MeasureTool, a new TextureLayer class, the LayerManager, and others. To see all painting objects owned by MarbleMap, see the modified MarbleMap_p.h.
Since the LayerManager object is now owned by MarbleMap, the layer-relevant methods, slots, and signals (such as addLayer(), removeLayer(), renderPluginsInitialized()) are now forwarded by MarbleMap and Marblewidget rather than by MarbleModel.
The DownloadRegionDialog has been modified to take a MarbleWidget instead of a MarbleModel. I wonder if it makes more sense to take a MarbleMap here.
Please test if the patch works for you as well. Since the patch changes some initializaton orders in the constructors and some re-initialization orders on theme change, it is important to test these code paths more extensively.
If you see that the patch violates some assumptions/invariants, please report as well.</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;">Works for me using the KDE version in the majority of cases.</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/ControlView.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleMap_p.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetPopupMenu.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.h <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/marble_part.cpp <span style="color: grey">(1198738)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plasmoid/worldclock.cpp <span style="color: grey">(1198738)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5902/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>