<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2010, 12:11 a.m., <b>Thibaut Gridel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A first look at the patch, excellent work. The hidden comment "This function will probably move to MarbleWidget in KDE4.1, making the MarbleModel/MarbleWidget pair truly follow the Model/View paradigm" is long overdue!</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for your comment!</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2010, 12:11 a.m., <b>Thibaut Gridel</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="http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41472#file41472line1139" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/QtMainWindow.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 MainWindow::showDownloadRegionDialog()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1139</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">MarbleModel</span> <span class="o">*</span><span class="k">const</span> <span class="n">model</span> <span class="o">=</span> <span class="n">m_controlView</span><span class="o">-></span><span class="n">marbleWidget</span><span class="p">()</span><span class="o">-></span><span class="n">model</span><span class="p">();</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1139</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">MarbleModel</span> <span class="o">*</span><span class="k">const</span> <span class="n">model</span> <span class="o">=</span> <span class="n">m_controlView</span><span class="o">-></span><span class="n">marbleWidget</span><span class="p">()</span><span class="o">-></span><span class="n">model</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">model seems not used anymore here</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think you're right and I'll remove it after I have a look.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2010, 12:11 a.m., <b>Thibaut Gridel</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="http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41478#file41478line827" 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 MarbleMap::setMapThemeId( const QString& mapThemeId )</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">823</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Set all the colors for the vector layers</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In a later version, could be useful to have layers react directly to theme change.
For instance, veccomposer would update its properties in its own slot for setMapThemeIt</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">An alternative approach would be to create a new stack of layers on every theme change. That way, the layers wouldn't have to deal with theme changes at all. All this could be implemented in a map builder class which creates a stack of layers from a GeoSceneDocument. With a nice GUI on top we could even have a graphical map theme editor.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2010, 12:11 a.m., <b>Thibaut Gridel</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="http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41479#file41479line66" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleMap_p.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 MarbleMapPrivate</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">65</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">LayerManager</span> <span class="n">m_layerManager</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I guess most of this code was moved, so more changes could still wait a bit.
Is it expected to change to pointers here, at least for all layers so they could simply be added to the LayerManager, and have a consistent manipulation?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I agree that this patch should solve as few things at a time as possible. However, I wanted to make sure that the initialization sequence in the constructor is absolutely clear since that eases further refactoring significantly.
Having a common layer interface for all sorts of layers will be possible with the new layer interface which I want to postpone to 4.7 since I don't want to put more pressure on other developers.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2010, 12:11 a.m., <b>Thibaut Gridel</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="http://svn.reviewboard.kde.org/r/5902/diff/1/?file=41481#file41481line155" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.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; ">namespace Marble</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">147</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">StackedTileLoader</span> <span class="o">*</span><span class="n">m_tileLoader</span><span class="p">;</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; "></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Don't know enough of the StackedTileLoader to determine if it is more a model store or a layer rendering class...
Maybe this class could benefit from a split, with storing in model and rendering in map? (maybe it doesn't have sense to split so...)
Is it used in GL with either role?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The StackedTileLoader is responsible for merging/bleding tiles form different textures into one. As such, it is part of the graphical representation.
The split you mention is already there: There is one TileLoader which actually loads the tiles from different textures and passes them to the StackedTileLoader. The TileLoader currently lives inside the StackedTileLoader but I'd like to keep it this way in this patch since I don't want to increase the size and complexity of this patch even further. :)</pre>
<br />
<p>- Bernhard</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>