<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="https://git.reviewboard.kde.org/r/114729/">https://git.reviewboard.kde.org/r/114729/</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;">Can you include the newly created files in the diff in the future? Then it is possible to add notes to lines directly. If you use git diff to create diffs, you'll have to use git add to have files become of the diff (or work with branches and create commits).
for GeoDataSnippet.h:
Can you change the constructor from
explicit GeoDataSnippet( const QString &text , const int maxLines );
to
explicit GeoDataSnippet( const QString &text=QString() , const int maxLines=0 );
int maxLines();
should be
int maxLines() const;
same for
QString text();
should be
QString text() const;
Both QString m_text and int m_maxLines should be in the private: section.
Can you take a look at the #includes of the file and clean them up? Many of them shouldn't be needed.
</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="https://git.reviewboard.kde.org/r/114729/diff/4/?file=227951#file227951line299" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/data/GeoDataFeature.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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 GEODATA_EXPORT GeoDataFeature : public GeoDataObject</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">299</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * If a Snippet is not supplied, the first two lines of the <description> are used.</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;">Please remove the "If a snippet... not supported" part of the documentation: It's not implemented here like that, and shouldn't be as well.
Doing that might be nice further down when setting the content of the popup though. Maybe add a TODO comment there asking to implement that later on?
</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="https://git.reviewboard.kde.org/r/114729/diff/4/?file=227953#file227953line36" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/data/GeoDataFeature_p.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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="#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">36</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_snippet</span><span class="p">(</span><span class="s">""</span><span class="p">,</span> <span class="mi">1</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;">GeoDataSnippet should have a default constructor that does this (ideally setting maxLines=0 meaning no restriction is set) </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="https://git.reviewboard.kde.org/r/114729/diff/4/?file=227955#file227955line140" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/layers/PopupLayer.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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 PopupLayer::setUrl( const QUrl &url )</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">140</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">content</span> <span class="o">=</span> <span class="n">content</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="s">"$[snippet]"</span><span class="p">,</span> <span class="n">placemark</span><span class="o">-></span><span class="n">snippet</span><span class="p">().</span><span class="n">text</span><span class="p">(),</span> <span class="n">Qt</span><span class="o">::</span><span class="n">CaseInsensitive</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;">Here would be the right place to calculate a snippet implementing the behavior as noticed in the KML reference (see comment above), so I'd insert a TODO comment here.
</pre>
</div>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On December 30th, 2013, 9:22 a.m. UTC, Levente Kurusa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble, Dennis Nienhüser and Torsten Rahn.</div>
<div>By Levente Kurusa.</div>
<p style="color: grey;"><i>Updated Dec. 30, 2013, 9:22 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
marble
</div>
<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;">This is a GCI task.
Added handling of $[*] and the displayMode tag.</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/marble/layers/PopupLayer.cpp <span style="color: grey">(ff554d1)</span></li>
<li>src/lib/marble/layers/PopupLayer.h <span style="color: grey">(06c73c7)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature_p.h <span style="color: grey">(92de8a1)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature.cpp <span style="color: grey">(ba7d952)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature.h <span style="color: grey">(b54e3e5)</span></li>
<li>src/lib/marble/geodata/CMakeLists.txt <span style="color: grey">(0386159)</span></li>
<li>src/lib/marble/MarbleWidgetPopupMenu.cpp <span style="color: grey">(d6fb4a6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114729/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/29/b71132cb-f7b5-4c83-804c-82e0256fa00b__snapshot2.png">screenie</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/29/876f1585-3f6b-41a8-81bd-f8f883e00357__baloon.kml">KML used in screenie</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/4a85ae32-6088-4725-b38b-9f5565473e70__KmlSnippetTagHandler.cpp">KmlSnippetTagHandler</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/7651a627-ed5e-4ae3-8939-9bb2bc793101__KmlSnippetTagHandler.h">KmlSnippetTagHandler.h</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/5c00a6a8-117c-482b-b01f-61b1f78a23ba__GeoDataSnippet.cpp">GeoDataSnippet.cpp</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/06dbcca0-2647-45dc-9f70-b7797ed542d5__GeoDataSnippet.h">GeoDataSnippet.h</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>